[RISCV] When using global merging, don't enable merging of external globals by default #115484
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
…lobals by default AArch64 left this disabled after seeing some cases of slightly worse codegen that weren't tracked down, so I suggest as a path to incrementally moving towards enable globals merging we follow suit, and evaluate turning on later. This patch disables merging of external globals, but also adds a flag to override that. This reduces churn in test cases, simplifies benchmarking runs, and this flag can be removed later.
|
@llvm/pr-subscribers-backend-risc-v Author: Alex Bradbury (asb) ChangesAArch64 left this disabled after seeing some cases of slightly worse codegen that weren't tracked down, so I suggest as a path to incrementally moving towards enable globals merging we follow suit, and evaluate turning on later. This patch disables merging of external globals, but also adds a flag to override that. This reduces churn in test cases, simplifies benchmarking runs, and this flag can be removed later. A follow-on PR enables the globals merging pass by default (and as it's based on this commit, merging of external globals is disabled just as they are for AArch64). Full diff: https://github.com/llvm/llvm-project/pull/115484.diff 6 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
index a5afbcfd79710f..6a97755c279a29 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
@@ -53,6 +53,13 @@ static cl::opt<cl::boolOrDefault>
EnableGlobalMerge("riscv-enable-global-merge", cl::Hidden,
cl::desc("Enable the global merge pass"));
+static cl::opt<bool> ForceEnableGlobalMergeExternalGlobals(
+ "riscv-force-enable-global-merge-external-globals", cl::Hidden,
+ cl::init(false),
+ cl::desc(
+ "If the global merge pass is enabled, force enable global merging of "
+ "external globals (overriding any logic that might disable it)"));
+
static cl::opt<bool>
EnableMachineCombiner("riscv-enable-machine-combiner",
cl::desc("Enable the machine combiner pass"),
@@ -472,7 +479,8 @@ bool RISCVPassConfig::addPreISel() {
if (EnableGlobalMerge == cl::BOU_TRUE) {
addPass(createGlobalMergePass(TM, /* MaxOffset */ 2047,
/* OnlyOptimizeForSize */ false,
- /* MergeExternalByDefault */ true));
+ /* MergeExternalByDefault */
+ ForceEnableGlobalMergeExternalGlobals));
}
return false;
diff --git a/llvm/test/CodeGen/RISCV/global-merge-minsize-smalldata-nonzero.ll b/llvm/test/CodeGen/RISCV/global-merge-minsize-smalldata-nonzero.ll
index c5471389302124..39c677ac20b3a5 100644
--- a/llvm/test/CodeGen/RISCV/global-merge-minsize-smalldata-nonzero.ll
+++ b/llvm/test/CodeGen/RISCV/global-merge-minsize-smalldata-nonzero.ll
@@ -1,8 +1,9 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=riscv64 -riscv-enable-global-merge -verify-machineinstrs < %s \
-; RUN: | FileCheck %s -check-prefix=SMALL-DATA
+; RUN: llc -mtriple=riscv64 -riscv-enable-global-merge -riscv-force-enable-global-merge-external-globals \
+; RUN: -verify-machineinstrs < %s | FileCheck %s -check-prefix=SMALL-DATA
; RUN: llc -mtriple=riscv64 -riscv-enable-global-merge -global-merge-min-data-size=0 \
-; RUN: -verify-machineinstrs < %s | FileCheck %s -check-prefix=MINSIZE
+; RUN: -riscv-force-enable-global-merge-external-globals -verify-machineinstrs < %s \
+; RUN: | FileCheck %s -check-prefix=MINSIZE
@ig1 = internal global i32 0, align 4
@ig2 = internal global i32 0, align 4
diff --git a/llvm/test/CodeGen/RISCV/global-merge-minsize-smalldata-zero.ll b/llvm/test/CodeGen/RISCV/global-merge-minsize-smalldata-zero.ll
index 8e4d72af00ebce..d2b714577db9a8 100644
--- a/llvm/test/CodeGen/RISCV/global-merge-minsize-smalldata-zero.ll
+++ b/llvm/test/CodeGen/RISCV/global-merge-minsize-smalldata-zero.ll
@@ -1,8 +1,9 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=riscv32 -riscv-enable-global-merge -verify-machineinstrs < %s \
-; RUN: | FileCheck %s -check-prefix=SMALL-DATA
+; RUN: llc -mtriple=riscv32 -riscv-enable-global-merge -riscv-force-enable-global-merge-external-globals \
+; RUN: -verify-machineinstrs < %s | FileCheck %s -check-prefix=SMALL-DATA
; RUN: llc -mtriple=riscv64 -riscv-enable-global-merge -global-merge-min-data-size=5 \
-; RUN: -verify-machineinstrs < %s | FileCheck %s -check-prefix=MINSIZE
+; RUN: -riscv-force-enable-global-merge-external-globals -verify-machineinstrs < %s \
+; RUN: | FileCheck %s -check-prefix=MINSIZE
@ig1 = internal global i32 0, align 4
@ig2 = internal global i32 0, align 4
diff --git a/llvm/test/CodeGen/RISCV/global-merge-minsize.ll b/llvm/test/CodeGen/RISCV/global-merge-minsize.ll
index e405425832acbb..696d163bdcb2c4 100644
--- a/llvm/test/CodeGen/RISCV/global-merge-minsize.ll
+++ b/llvm/test/CodeGen/RISCV/global-merge-minsize.ll
@@ -1,8 +1,8 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=riscv32 -riscv-enable-global-merge -verify-machineinstrs < %s \
-; RUN: | FileCheck %s -check-prefix=RV32
+; RUN: llc -mtriple=riscv32 -riscv-enable-global-merge -riscv-force-enable-global-merge-external-globals \
+; RUN: -verify-machineinstrs < %s | FileCheck %s -check-prefix=RV32
; RUN: llc -mtriple=riscv32 -riscv-enable-global-merge -global-merge-min-data-size=5 \
-; RUN: -verify-machineinstrs < %s | FileCheck %s -check-prefix=RV32-MINSIZE
+; RUN: -riscv-force-enable-global-merge-external-globals -verify-machineinstrs < %s | FileCheck %s -check-prefix=RV32-MINSIZE
@ig1 = internal global i32 0, align 4
@ig2 = internal global i32 0, align 4
diff --git a/llvm/test/CodeGen/RISCV/global-merge-offset.ll b/llvm/test/CodeGen/RISCV/global-merge-offset.ll
index 13afcba181719e..0c0881ddf28737 100644
--- a/llvm/test/CodeGen/RISCV/global-merge-offset.ll
+++ b/llvm/test/CodeGen/RISCV/global-merge-offset.ll
@@ -1,12 +1,12 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: sed 's/ArrSize/100/g' %s | llc -mtriple=riscv32 -riscv-enable-global-merge \
-; RUN: -verify-machineinstrs | FileCheck %s
+; RUN: -riscv-force-enable-global-merge-external-globals -verify-machineinstrs | FileCheck %s
; RUN: sed 's/ArrSize/100/g' %s | llc -mtriple=riscv64 -riscv-enable-global-merge \
-; RUN: -verify-machineinstrs | FileCheck %s
+; RUN: -riscv-force-enable-global-merge-external-globals -verify-machineinstrs | FileCheck %s
; RUN: sed 's/ArrSize/101/g' %s | llc -mtriple=riscv32 -riscv-enable-global-merge \
-; RUN: -verify-machineinstrs | FileCheck %s -check-prefix=CHECK-TOOBIG
+; RUN: -riscv-force-enable-global-merge-external-globals -verify-machineinstrs | FileCheck %s -check-prefix=CHECK-TOOBIG
; RUN: sed 's/ArrSize/101/g' %s | llc -mtriple=riscv64 -riscv-enable-global-merge \
-; RUN: -verify-machineinstrs | FileCheck %s -check-prefix=CHECK-TOOBIG
+; RUN: -riscv-force-enable-global-merge-external-globals -verify-machineinstrs | FileCheck %s -check-prefix=CHECK-TOOBIG
; This test demonstrates that the MaxOffset is set correctly for RISC-V by
; constructing an input that is at the limit and comparing.
diff --git a/llvm/test/CodeGen/RISCV/global-merge.ll b/llvm/test/CodeGen/RISCV/global-merge.ll
index 20379ee2e7dacd..633ba719c6a305 100644
--- a/llvm/test/CodeGen/RISCV/global-merge.ll
+++ b/llvm/test/CodeGen/RISCV/global-merge.ll
@@ -3,6 +3,12 @@
; RUN: | FileCheck %s
; RUN: llc -mtriple=riscv64 -riscv-enable-global-merge -verify-machineinstrs < %s \
; RUN: | FileCheck %s
+; RUN: llc -mtriple=riscv32 -riscv-enable-global-merge \
+; RUN: -riscv-force-enable-global-merge-external-globals -verify-machineinstrs < %s \
+; RUN: | FileCheck -check-prefix=CHECK-WEXTERN %s
+; RUN: llc -mtriple=riscv64 -riscv-enable-global-merge \
+; RUN: -riscv-force-enable-global-merge-external-globals -verify-machineinstrs < %s \
+; RUN: | FileCheck -check-prefix=CHECK-WEXTERN %s
@ig1 = internal global i32 0, align 4
@ig2 = internal global i32 0, align 4
@@ -21,9 +27,21 @@ define void @f1(i32 %a) nounwind {
; CHECK-NEXT: sw a0, %lo(.L_MergedGlobals)(a1)
; CHECK-NEXT: addi a1, a1, %lo(.L_MergedGlobals)
; CHECK-NEXT: sw a0, 4(a1)
-; CHECK-NEXT: sw a0, 8(a1)
-; CHECK-NEXT: sw a0, 12(a1)
+; CHECK-NEXT: lui a1, %hi(eg1)
+; CHECK-NEXT: sw a0, %lo(eg1)(a1)
+; CHECK-NEXT: lui a1, %hi(eg2)
+; CHECK-NEXT: sw a0, %lo(eg2)(a1)
; CHECK-NEXT: ret
+;
+; CHECK-WEXTERN-LABEL: f1:
+; CHECK-WEXTERN: # %bb.0:
+; CHECK-WEXTERN-NEXT: lui a1, %hi(.L_MergedGlobals)
+; CHECK-WEXTERN-NEXT: sw a0, %lo(.L_MergedGlobals)(a1)
+; CHECK-WEXTERN-NEXT: addi a1, a1, %lo(.L_MergedGlobals)
+; CHECK-WEXTERN-NEXT: sw a0, 4(a1)
+; CHECK-WEXTERN-NEXT: sw a0, 8(a1)
+; CHECK-WEXTERN-NEXT: sw a0, 12(a1)
+; CHECK-WEXTERN-NEXT: ret
store i32 %a, ptr @ig1, align 4
store i32 %a, ptr @ig2, align 4
store i32 %a, ptr @eg1, align 4
|
asb
added a commit
to asb/llvm-project
that referenced
this pull request
Nov 8, 2024
Stacks on top of <llvm#115484>. From the discussion at the round-table at the RISC-V Summit it was clear people see cases where global merging would help. So the direction of enabling it by default and iteratively working to enable it in more cases or to improve the heuristics seems sensible. This patch tries to make a minimal step in that direction.
Groverkss
pushed a commit
to iree-org/llvm-project
that referenced
this pull request
Nov 15, 2024
…lobals by default (llvm#115484) AArch64 left this disabled after seeing some cases of slightly worse codegen that weren't tracked down, so I suggest as a path to incrementally moving towards enable globals merging we follow suit, and evaluate turning on later. This patch disables merging of external globals, but also adds a flag to override that. This reduces churn in test cases, simplifies benchmarking runs, and this flag can be removed later. A follow-on PR enables the globals merging pass by default (and as it's based on this commit, merging of external globals is disabled just as they are for AArch64).
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.
AArch64 left this disabled after seeing some cases of slightly worse codegen that weren't tracked down, so I suggest as a path to incrementally moving towards enable globals merging we follow suit, and evaluate turning on later.
This patch disables merging of external globals, but also adds a flag to override that. This reduces churn in test cases, simplifies benchmarking runs, and this flag can be removed later.
A follow-on PR enables the globals merging pass by default (and as it's based on this commit, merging of external globals is disabled just as they are for AArch64).