[GlobalMerge] Convert GlobalMerge to a ModulePass #160703
Open
+18
−15
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GlobalMerge is written as a FunctionPass, yet does all of its work in initialisation. This might technically mean that it doesn't break the invariant that function passes shouldn't add/remove globals, but it's very surprising and means that attempts in e.g. the AArch64 backend to schedule global merging after a constant promotion pass doesn't actually work as expected. This issue was previously noted in <llvm#89708 (comment)>. Going through commit history I was unable to find a justification for why it is written this way, so I will flag to reviewers that it's possible I'm missing something.
|
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-backend-powerpc Author: Alex Bradbury (asb) ChangesGlobalMerge is written as a FunctionPass, yet does all of its work in initialisation. This might technically mean that it doesn't break the invariant that function passes shouldn't add/remove globals, but it's very surprising and means that attempts in e.g. the AArch64 backend to schedule global merging after a constant promotion pass doesn't actually work as expected. This issue was previously noted in <#89708 (comment)>. Going through commit history I was unable to find a justification for why it is written this way, so I will flag to reviewers that it's possible I'm missing something. Full diff: https://github.com/llvm/llvm-project/pull/160703.diff 6 Files Affected:
diff --git a/llvm/lib/CodeGen/GlobalMerge.cpp b/llvm/lib/CodeGen/GlobalMerge.cpp
index e58d7e344c28b..251bbcb90089f 100644
--- a/llvm/lib/CodeGen/GlobalMerge.cpp
+++ b/llvm/lib/CodeGen/GlobalMerge.cpp
@@ -189,14 +189,14 @@ class GlobalMergeImpl {
bool run(Module &M);
};
-class GlobalMerge : public FunctionPass {
+class GlobalMerge : public ModulePass {
const TargetMachine *TM = nullptr;
GlobalMergeOptions Opt;
public:
static char ID; // Pass identification, replacement for typeid.
- explicit GlobalMerge() : FunctionPass(ID) {
+ explicit GlobalMerge() : ModulePass(ID) {
Opt.MaxOffset = GlobalMergeMaxOffset;
Opt.MergeConstantGlobals = EnableGlobalMergeOnConst;
Opt.MergeConstAggressive = GlobalMergeAllConst;
@@ -206,7 +206,7 @@ class GlobalMerge : public FunctionPass {
explicit GlobalMerge(const TargetMachine *TM, unsigned MaximalOffset,
bool OnlyOptimizeForSize, bool MergeExternalGlobals,
bool MergeConstantGlobals, bool MergeConstAggressive)
- : FunctionPass(ID), TM(TM) {
+ : ModulePass(ID), TM(TM) {
Opt.MaxOffset = MaximalOffset;
Opt.SizeOnly = OnlyOptimizeForSize;
Opt.MergeExternal = MergeExternalGlobals;
@@ -215,7 +215,7 @@ class GlobalMerge : public FunctionPass {
initializeGlobalMergePass(*PassRegistry::getPassRegistry());
}
- bool doInitialization(Module &M) override {
+ bool runOnModule(Module &M) override {
auto GetSmallDataLimit = [](Module &M) -> std::optional<uint64_t> {
Metadata *SDL = M.getModuleFlag("SmallDataLimit");
if (!SDL)
@@ -232,13 +232,11 @@ class GlobalMerge : public FunctionPass {
GlobalMergeImpl P(TM, Opt);
return P.run(M);
}
- bool runOnFunction(Function &F) override { return false; }
StringRef getPassName() const override { return "Merge internal globals"; }
void getAnalysisUsage(AnalysisUsage &AU) const override {
AU.setPreservesCFG();
- FunctionPass::getAnalysisUsage(AU);
}
};
diff --git a/llvm/test/CodeGen/AArch64/O3-pipeline.ll b/llvm/test/CodeGen/AArch64/O3-pipeline.ll
index e1481667a4ab7..9353fdfb8d09b 100644
--- a/llvm/test/CodeGen/AArch64/O3-pipeline.ll
+++ b/llvm/test/CodeGen/AArch64/O3-pipeline.ll
@@ -105,8 +105,8 @@
; CHECK-NEXT: AArch64 Promote Constant
; CHECK-NEXT: FunctionPass Manager
; CHECK-NEXT: Dominator Tree Construction
+; CHECK-NEXT: Merge internal globals
; CHECK-NEXT: FunctionPass Manager
-; CHECK-NEXT: Merge internal globals
; CHECK-NEXT: Dominator Tree Construction
; CHECK-NEXT: Basic Alias Analysis (stateless AA impl)
; CHECK-NEXT: Function Alias Analysis Results
diff --git a/llvm/test/CodeGen/ARM/O3-pipeline.ll b/llvm/test/CodeGen/ARM/O3-pipeline.ll
index 9601a2e4e3d12..b7d47c6320a55 100644
--- a/llvm/test/CodeGen/ARM/O3-pipeline.ll
+++ b/llvm/test/CodeGen/ARM/O3-pipeline.ll
@@ -55,7 +55,9 @@
; CHECK-NEXT: CodeGen Prepare
; CHECK-NEXT: Dominator Tree Construction
; CHECK-NEXT: Exception handling preparation
-; CHECK-NEXT: Merge internal globals
+; CHECK-NEXT: Merge internal globals
+; CHECK-NEXT: FunctionPass Manager
+; CHECK-NEXT: Dominator Tree Construction
; CHECK-NEXT: Natural Loop Information
; CHECK-NEXT: Scalar Evolution Analysis
; CHECK-NEXT: Lazy Branch Probability Analysis
@@ -64,8 +66,8 @@
; CHECK-NEXT: Hardware Loop Insertion
; CHECK-NEXT: Loop Pass Manager
; CHECK-NEXT: Transform predicated vector loops to use MVE tail predication
-; CHECK-NEXT: A No-Op Barrier Pass
-; CHECK-NEXT: FunctionPass Manager
+; CHECK-NEXT: A No-Op Barrier Pass
+; CHECK-NEXT: FunctionPass Manager
; CHECK-NEXT: Dominator Tree Construction
; CHECK-NEXT: Basic Alias Analysis (stateless AA impl)
; CHECK-NEXT: Function Alias Analysis Results
@@ -111,7 +113,7 @@
; CHECK-NEXT: Modulo Software Pipelining
; CHECK-NEXT: MachineDominator Tree Construction
; CHECK-NEXT: Machine Natural Loop Construction
-; CHECK-NEXT: MVE TailPred and VPT Optimisation Pass
+; CHECK-NEXT: ARM MVE TailPred and VPT Optimisation Pass
; CHECK-NEXT: ARM MLA / MLS expansion pass
; CHECK-NEXT: MachineDominator Tree Construction
; CHECK-NEXT: ARM pre- register allocation load / store optimization pass
diff --git a/llvm/test/CodeGen/PowerPC/O3-pipeline.ll b/llvm/test/CodeGen/PowerPC/O3-pipeline.ll
index 7cbb1a1c98873..be044daa1fe2e 100644
--- a/llvm/test/CodeGen/PowerPC/O3-pipeline.ll
+++ b/llvm/test/CodeGen/PowerPC/O3-pipeline.ll
@@ -72,7 +72,9 @@
; CHECK-NEXT: CodeGen Prepare
; CHECK-NEXT: Dominator Tree Construction
; CHECK-NEXT: Exception handling preparation
-; CHECK-NEXT: Merge internal globals
+; CHECK-NEXT: Merge internal globals
+; CHECK-NEXT: FunctionPass Manager
+; CHECK-NEXT: Dominator Tree Construction
; CHECK-NEXT: Natural Loop Information
; CHECK-NEXT: Scalar Evolution Analysis
; CHECK-NEXT: Prepare loop for ppc preferred instruction forms
diff --git a/llvm/test/CodeGen/PowerPC/mergeable-string-pool-exceptions.ll b/llvm/test/CodeGen/PowerPC/mergeable-string-pool-exceptions.ll
index 10179bba136f9..ea8e75ca46dd7 100644
--- a/llvm/test/CodeGen/PowerPC/mergeable-string-pool-exceptions.ll
+++ b/llvm/test/CodeGen/PowerPC/mergeable-string-pool-exceptions.ll
@@ -15,8 +15,9 @@ define ptr @test1() personality ptr @__gnu_objc_personality_v0 {
; CHECK-NEXT: std 0, 48(1)
; CHECK-NEXT: .cfi_def_cfa_offset 32
; CHECK-NEXT: .cfi_offset lr, 16
-; CHECK-NEXT: addis 3, 2, .Ldummy@toc@ha
-; CHECK-NEXT: addi 3, 3, .Ldummy@toc@l
+; CHECK-NEXT: addis 3, 2, .L_MergedGlobals@toc@ha
+; CHECK-NEXT: addi 3, 3, .L_MergedGlobals@toc@l
+; CHECK-NEXT: addi 3, 3, 4
; CHECK-NEXT: bl foo
; CHECK-NEXT: nop
invoke void @foo(ptr @dummy)
diff --git a/llvm/test/CodeGen/RISCV/O3-pipeline.ll b/llvm/test/CodeGen/RISCV/O3-pipeline.ll
index ea08061221fd4..0e42ad319b8dd 100644
--- a/llvm/test/CodeGen/RISCV/O3-pipeline.ll
+++ b/llvm/test/CodeGen/RISCV/O3-pipeline.ll
@@ -76,8 +76,8 @@
; CHECK-NEXT: Dominator Tree Construction
; CHECK-NEXT: Exception handling preparation
; CHECK-NEXT: A No-Op Barrier Pass
+; CHECK-NEXT: Merge internal globals
; CHECK-NEXT: FunctionPass Manager
-; CHECK-NEXT: Merge internal globals
; CHECK-NEXT: Dominator Tree Construction
; CHECK-NEXT: Basic Alias Analysis (stateless AA impl)
; CHECK-NEXT: Function Alias Analysis Results
|
This pass must run before |
Thank you for clarifying why it's written the way it is! I'm seeing failures I don't see locally in the pre-commit CI build, which indicates I got lucky with the order of doInitialization locally. Have you looked at all on whether it would be possible to remove this dependency?
The comment can be removed for sure, but it's still a case of an in-tree user of the pass hoping they can have GlobalMerge run to clean up globals emitted by a previous pass. I came across this behaviour when prototyping a pass and looking to use GlobalMerge in the same way. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GlobalMerge is written as a FunctionPass, yet does all of its work in initialisation. This might technically mean that it doesn't break the invariant that function passes shouldn't add/remove globals, but it's very surprising and means that attempts in e.g. the AArch64 backend to schedule global merging after a constant promotion pass doesn't actually work as expected. This issue was previously noted in #89708 (comment).
Going through commit history I was unable to find a justification for why it is written this way, so I will flag to reviewers that it's possible I'm missing something.