Skip to content

Conversation

@jayfoad
Copy link
Contributor

@jayfoad jayfoad commented Nov 28, 2024

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Nov 28, 2024

@llvm/pr-subscribers-backend-amdgpu

Author: Jay Foad (jayfoad)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/117994.diff

4 Files Affected:

  • (modified) llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp (+2-2)
  • (modified) llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp (+3-1)
  • (modified) llvm/lib/Target/AMDGPU/AMDGPULateCodeGenPrepare.cpp (+2-2)
  • (modified) llvm/lib/Target/AMDGPU/SIAnnotateControlFlow.cpp (+1-1)
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp b/llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp
index 5feb35a2b553a5..9da0d93341ddfd 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp
@@ -92,10 +92,10 @@ AMDGPUAnnotateUniformValuesPass::run(Function &F,
   AMDGPUAnnotateUniformValues Impl(UI, MSSA, AA, F);
   Impl.visit(F);
 
-  PreservedAnalyses PA = PreservedAnalyses::none();
   if (!Impl.changed())
-    return PA;
+    return PreservedAnalyses::all();
 
+  PreservedAnalyses PA = PreservedAnalyses::none();
   // TODO: Should preserve nearly everything
   PA.preserveSet<CFGAnalyses>();
   return PA;
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp b/llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp
index c49aab823b44a4..7257b53afe69d0 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp
@@ -2303,10 +2303,12 @@ PreservedAnalyses AMDGPUCodeGenPreparePass::run(Function &F,
   SIModeRegisterDefaults Mode(F, *Impl.ST);
   Impl.HasFP32DenormalFlush =
       Mode.FP32Denormals == DenormalMode::getPreserveSign();
+  if (!Impl.run(F))
+    return PreservedAnalyses::all();
   PreservedAnalyses PA = PreservedAnalyses::none();
   if (!Impl.FlowChanged)
     PA.preserveSet<CFGAnalyses>();
-  return Impl.run(F) ? PA : PreservedAnalyses::all();
+  return PA;
 }
 
 INITIALIZE_PASS_BEGIN(AMDGPUCodeGenPrepare, DEBUG_TYPE,
diff --git a/llvm/lib/Target/AMDGPU/AMDGPULateCodeGenPrepare.cpp b/llvm/lib/Target/AMDGPU/AMDGPULateCodeGenPrepare.cpp
index 30ef390faab8bd..86ed29acb09abc 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPULateCodeGenPrepare.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPULateCodeGenPrepare.cpp
@@ -484,9 +484,9 @@ AMDGPULateCodeGenPreparePass::run(Function &F, FunctionAnalysisManager &FAM) {
 
   bool Changed = Impl.run(F);
 
-  PreservedAnalyses PA = PreservedAnalyses::none();
   if (!Changed)
-    return PA;
+    return PreservedAnalyses::all();
+  PreservedAnalyses PA = PreservedAnalyses::none();
   PA.preserveSet<CFGAnalyses>();
   return PA;
 }
diff --git a/llvm/lib/Target/AMDGPU/SIAnnotateControlFlow.cpp b/llvm/lib/Target/AMDGPU/SIAnnotateControlFlow.cpp
index ea653490f1bf37..67012669a6df0c 100644
--- a/llvm/lib/Target/AMDGPU/SIAnnotateControlFlow.cpp
+++ b/llvm/lib/Target/AMDGPU/SIAnnotateControlFlow.cpp
@@ -391,7 +391,7 @@ PreservedAnalyses SIAnnotateControlFlowPass::run(Function &F,
   // FIXME: We introduce dead declarations of intrinsics even if never used.
   bool Changed = Impl.run(F);
   if (!Changed)
-    return PreservedAnalyses::none();
+    return PreservedAnalyses::all();
 
   // TODO: Is LoopInfo preserved?
   PreservedAnalyses PA = PreservedAnalyses::none();

@jayfoad jayfoad requested review from arsenm and gandhi56 and removed request for arsenm November 28, 2024 11:32
Copy link
Contributor Author

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found by code inspection.

SIModeRegisterDefaults Mode(F, *Impl.ST);
Impl.HasFP32DenormalFlush =
Mode.FP32Denormals == DenormalMode::getPreserveSign();
if (!Impl.run(F))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code was checking Impl.FlowChanged before calling Impl.run which seems completely wrong.

@jayfoad jayfoad merged commit 3923e04 into llvm:main Nov 28, 2024
10 checks passed
@jayfoad jayfoad deleted the preserve-all branch November 28, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants