[Uniformity] Legacy PM: Set UniformityInfoWrapperPass isCFGOnly to false

[J-MR-T]

Currently, Uniformity Analysis is preserved in the Legacy PM when a pass sets setPreservesCFG(). This is incorrect: any values' uniformity not only depends on the CFG, but also on the uniformity of other values, so a CFG-preserving change in many cases doesn't preserve uniformity analysis.

This corrected behavior also matches the behavior of the new PM.

---

On its own, this change does not affect the pass pipeline because of the happenstance of pass ordering. I also created a PR to change AMDGPULateCodeGenPrepare (#148167), this will have an actual impact on pass executions. That PR also includes changes to the amdgpu/llc-pipeline.ll test in order to check that this change works, but if this is preferred, I would also be happy to try to extend this PR to add an isolated test case; though my personal opinion is that the test in #148167 should suffice, as it should also accurately pinpoint failures related to this change.

---

I ran git-clang-format on my changes. I tested them using the check-llvm target; no unexpected failures occurred.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Jim M. R. Teichgräber (J-MR-T)

Changes

Currently, Uniformity Analysis is preserved in the Legacy PM when a pass sets setPreservesCFG(). This is incorrect: any values' uniformity not only depends on the CFG, but also on the uniformity of other values, so a CFG-preserving change in many cases doesn't preserve uniformity analysis.

This corrected behavior also matches the behavior of the new PM.

---

On its own, this change does not affect the pass pipeline because of the happenstance of pass ordering. In a minute, I'll also create a PR to change AMDGPULateCodeGenPrepare and link this here, this will have an actual impact on pass executions. That PR will also include changes to the amdgpu/llc-pipeline.ll test in order to check that this change works, but if this is preferred, I would also be happy to try to extend this PR to add an isolated test case; though my personal opinion is that the test in the other PR should suffice, as it should also accurately pinpoint failures related to this change.

---

I ran git-clang-format on my changes. I tested them using the check-llvm target; no unexpected failures occurred.

---

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

1 Files Affected:

• (modified) llvm/lib/Analysis/UniformityAnalysis.cpp (+4-1)

diff --git a/llvm/lib/Analysis/UniformityAnalysis.cpp b/llvm/lib/Analysis/UniformityAnalysis.cpp
index 2101fdfacfc8f..0daf4041a72f3 100644
--- a/llvm/lib/Analysis/UniformityAnalysis.cpp
+++ b/llvm/lib/Analysis/UniformityAnalysis.cpp
@@ -150,8 +150,11 @@ INITIALIZE_PASS_BEGIN(UniformityInfoWrapperPass, "uniformity",
 INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(CycleInfoWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(TargetTransformInfoWrapperPass)
+// Though Uniformity Analysis depends on the CFG,
+// it also needs to be invalidated if values are changed, so isCFGOnly: false.
+// See NOTE on updatability at the start of GenericUniformityImpl.h
 INITIALIZE_PASS_END(UniformityInfoWrapperPass, "uniformity",
-                    "Uniformity Analysis", true, true)
+                    "Uniformity Analysis", false, true)
 
 void UniformityInfoWrapperPass::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.setPreservesAll();

[J-MR-T]

CC @nhaehnle

[nhaehnle]

The BEGIN macro use should also be updated.

And personally, I'd say you should drop the comment. It only make sense to me with the historical context, which isn't visible when reading the post-change version of the code.

[J-MR-T]

The BEGIN macro use should also be updated.

Oops, I didn't notice that that bool is duplicated there (my clangd only seems to show an inlay hint for the END version of the macro, as the BEGIN cfg/analysis arguments don't seem to be used).

And personally, I'd say you should drop the comment.

I was also on the fence on that comment, so I'm also happy to get rid of it :)

[J-MR-T]

Sidenote: Is there a reason that INITIALIZE_PASS_BEGIN has the arg, name, cfg, and analysis arguments?

llvm-project/llvm/include/llvm/PassSupport.h

Lines 39 to 40 in b152611

	#define INITIALIZE_PASS_BEGIN(passName, arg, name, cfg, analysis) \
	static void initialize##passName##PassOnce(PassRegistry &Registry) {

It seems redundant.

[nhaehnle]

Sidenote: Is there a reason that INITIALIZE_PASS_BEGIN has the arg, name, cfg, and analysis arguments?

llvm-project/llvm/include/llvm/PassSupport.h

Lines 39 to 40 in b152611

	#define INITIALIZE_PASS_BEGIN(passName, arg, name, cfg, analysis) \
	static void initialize##passName##PassOnce(PassRegistry &Registry) {

It seems redundant.

I assume this was done for symmetry and in case the implementation of the macros would ever have to change.

I don't know if you have write access, so I took the liberty of merging it.