[RISCV] Add the GlobalMerge pass (disabled by default)

[asb]

Summary

Split out from D129178, this just adds the GlobalMerge tests (other than global-merge-minsize.ll which is testing a specific configuration of the pass when it's enabled) and exposes -riscv-enable-global-merge and doesn't enable it by default.

Note that the comment "// FIXME: Unify control over GlobalMerge." is copied from the Arm and AArch64 backends, which expose the same flag. Presumably the author is imagining some later refactoring that provides a target-independent flag.

[reames]

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

47

Please add the explicit cl::init(false) rather than relying on default initialization.

213

Can't this just be if (EnableGlobalMerge)?

llvm/test/CodeGen/RISCV/global-merge-offset.ll

2

The use of sed here is simply overly complicating things.

Please add a second test to this file which uses a separate global with an alternate size.

[luismarques]

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

213

This seems to be used in various other places.

[craig.topper]

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

47

It's not relying on default initialization. It's a cl::boolOrDefault option not a bool option. This due to the usage in the original patch where it has 3 behaviors.

213

It's not a bool option it's boolOrDefault option so it has 3 values not 2.

[asb]

Re EnableGlobalMerge - I'm happy to make it just a bool option for this patch if that's preferable, but the follow-on patch that enables it should really change it back to the tri-value boolOrDefault in my opinion in order to match the equivalent option on AArch64 and Arm (and because the logic takes advantage of all three values). Let me know what you think.

llvm/test/CodeGen/RISCV/global-merge-offset.ll

2

The use of sed is because it's not very easy to construct a test (in a way that remains easily readable at least) that demonstrates the global merging behaviour.

The file I started with did:

@ga1 = dso_local global [510 x i32] zeroinitializer, align 4
@gi1 = dso_local global i32 0, align 4

; TODO: It would be better for codesize if the final store below was
; `sw a0, 0(a2)`.

define void @f_mergeable(i32 %a) nounwind {
; CHECK-LABEL: f_mergeable:
; CHECK:       # %bb.0:
; CHECK-NEXT:    lui a1, %hi(ga1+2040)
; CHECK-NEXT:    sw a0, %lo(ga1+2040)(a1)
; CHECK-NEXT:    lui a1, %hi(.L_MergedGlobals)
; CHECK-NEXT:    sw a0, %lo(.L_MergedGlobals)(a1)
; CHECK-NEXT:    ret
  %ga1_end = getelementptr inbounds [510 x i32], ptr @ga1, i32 0, i64 510
  store i32 %a, ptr %ga1_end, align 4
  store i32 %a, ptr @gi1, align 4
  ret void
}

@ga2 = dso_local global [511 x i32] zeroinitializer, align 4
@gi2 = dso_local global i32 0, align 4

define void @f_unmergeable(i32 %a) nounwind {
; CHECK-LABEL: f_unmergeable:
; CHECK:       # %bb.0:
; CHECK-NEXT:    lui a1, %hi(ga2+2040)
; CHECK-NEXT:    sw a0, %lo(ga2+2040)(a1)
; CHECK-NEXT:    lui a1, %hi(.L_MergedGlobals+4)
; CHECK-NEXT:    sw a0, %lo(.L_MergedGlobals+4)(a1)
; CHECK-NEXT:    ret
  %ga2_end = getelementptr inbounds [511 x i32], ptr @ga2, i32 0, i64 510
  store i32 %a, ptr %ga2_end, align 4
  store i32 %a, ptr @gi2, align 4
  ret void
}

But you can see the globals merging pass merged gi1 and gi2, making the test not so useful (room for improved heuristics in the pass too perhaps...).

[craig.topper]

llvm/test/CodeGen/RISCV/global-merge-offset.ll

2

So the options are sed or two test files? @reames which do you prefer?

[asb]

llvm/test/CodeGen/RISCV/global-merge-offset.ll

2

Yes, I think those are the feasible options.

[reames]

LGTM

On the test question, decided I didn't really care. I think we probably could construct this without sed, but the marginal improvement in readability isn't worth further delay. I might take a shot at a follow up patch on that issue specifically, or I might not.

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

47

I'm not familiar with usage of boolOrDefault. You can ignore my comment here if you believe boolOrDefault is a reasonable choice here. I'll defer to your judgement.

[hiraditya]

Thanks for enabling this!

[hiraditya]

is it okay to merge this patch or are there pending dependencies?

[craig.topper]

LGTM

[hiraditya]

LGTM

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

 #include "llvm/Transforms/IPO.h"
 using namespace llvm;
 
 static cl::opt<bool> EnableRedundantCopyElimination(
     "riscv-enable-copyelim",
     cl::desc("Enable the redundant copy elimination pass"), cl::init(true),
     cl::Hidden);
 
+// FIXME: Unify control over GlobalMerge.
+static cl::opt<cl::boolOrDefault>
+    EnableGlobalMerge("riscv-enable-global-merge", cl::Hidden,
+                      cl::desc("Enable the global merge pass"));
+
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeRISCVTarget() {
   RegisterTargetMachine<RISCVTargetMachine> X(getTheRISCV32Target());
   RegisterTargetMachine<RISCVTargetMachine> Y(getTheRISCV64Target());
   auto *PR = PassRegistry::getPassRegistry();
   initializeGlobalISel(*PR);
   initializeRISCVMakeCompressibleOptPass(*PR);
   initializeRISCVGatherScatterLoweringPass(*PR);
   initializeRISCVCodeGenPreparePass(*PR);

 
 bool RISCVPassConfig::addPreISel() {
   if (TM->getOptLevel() != CodeGenOpt::None) {
     // Add a barrier before instruction selection so that we will not get
     // deleted block address after enabling default outlining. See D99707 for
     // more details.
     addPass(createBarrierNoopPass());
   }
+
+  if (EnableGlobalMerge == cl::BOU_TRUE) {
+    addPass(createGlobalMergePass(TM, /* MaxOffset */ 2047,
+                                  /* OnlyOptimizeForSize */ false,
+                                  /* MergeExternalByDefault */ true));
+  }
+
   return false;
 }
 
 bool RISCVPassConfig::addInstSelector() {
   addPass(createRISCVISelDag(getRISCVTargetMachine(), getOptLevel()));
 
   return false;
 }

llvm/test/CodeGen/RISCV/global-merge-offset.ll

+; 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: sed 's/ArrSize/100/g' %s | llc -mtriple=riscv64 -riscv-enable-global-merge \
+; RUN:   -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: sed 's/ArrSize/101/g' %s | llc -mtriple=riscv64 -riscv-enable-global-merge \
+; RUN:   -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.
+
+@ga1 = dso_local global [410 x i32] zeroinitializer, align 4
+@ga2 = dso_local global [ArrSize x i32] zeroinitializer, align 4
+@gi = dso_local global i32 0, align 4
+
+; TODO: It would be better for codesize if the final store below was
+; `sw a0, 0(a2)`.
+
+define void @f1(i32 %a) nounwind {
+; CHECK-LABEL: f1:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    lui a1, %hi(.L_MergedGlobals)
+; CHECK-NEXT:    addi a2, a1, %lo(.L_MergedGlobals)
+; CHECK-NEXT:    sw a0, 2044(a2)
+; CHECK-NEXT:    sw a0, 404(a2)
+; CHECK-NEXT:    sw a0, %lo(.L_MergedGlobals)(a1)
+; CHECK-NEXT:    ret
+;
+; CHECK-TOOBIG-LABEL: f1:
+; CHECK-TOOBIG:       # %bb.0:
+; CHECK-TOOBIG-NEXT:    lui a1, %hi(ga1+1640)
+; CHECK-TOOBIG-NEXT:    lui a2, %hi(.L_MergedGlobals)
+; CHECK-TOOBIG-NEXT:    addi a3, a2, %lo(.L_MergedGlobals)
+; CHECK-TOOBIG-NEXT:    sw a0, %lo(ga1+1640)(a1)
+; CHECK-TOOBIG-NEXT:    sw a0, 408(a3)
+; CHECK-TOOBIG-NEXT:    sw a0, %lo(.L_MergedGlobals)(a2)
+; CHECK-TOOBIG-NEXT:    ret
+  %ga1_end = getelementptr inbounds [410 x i32], ptr @ga1, i32 0, i64 410
+  %ga2_end = getelementptr inbounds [ArrSize x i32], ptr @ga2, i32 0, i64 ArrSize
+  store i32 %a, ptr %ga1_end, align 4
+  store i32 %a, ptr %ga2_end, align 4
+  store i32 %a, ptr @gi, align 4
+  ret void
+}

llvm/test/CodeGen/RISCV/global-merge.ll

+; 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
+; RUN: llc -mtriple=riscv64 -riscv-enable-global-merge -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s
+
+@ig1 = internal global i32 0, align 4
+@ig2 = internal global i32 0, align 4
+
+@eg1 = dso_local global i32 0, align 4
+@eg2 = dso_local global i32 0, align 4
+
+; TODO: It would be better for code size to alter the first store below by
+; first fully materialising .L_MergedGlobals in a1 and then storing to it with
+; a 0 offset.
+
+define void @f1(i32 %a) nounwind {
+; CHECK-LABEL: f1:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    lui a1, %hi(.L_MergedGlobals)
+; 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:    ret
+  store i32 %a, ptr @ig1, align 4
+  store i32 %a, ptr @ig2, align 4
+  store i32 %a, ptr @eg1, align 4
+  store i32 %a, ptr @eg2, align 4
+  ret void
+}