Add options for PPC to enable/disable using non-volatile CR

[NeHuang]

Summary

In this patch, an option is added for PowerPC to disable using non-volatile CR and avoid CR spilling in the prologue. According to performance analysis results, we found performance gain for a few SPEC2017 benchmarks without introducing significant degradation.

[Yi-Hong.Lyu]

llvm/lib/Target/PowerPC/PPCRegisterInfo.td

368

Why do we need to check isELFv2ABI()?

[NeHuang]

@Yi-Hong.Lyu For PowerPC, CR2, CR3, CR4 are non-volatile CR fields according to ELFv2ABI, which will not hold for other ABIs.

[steven.zhang]

I didn't take deep look at the detail. But I think, it is better to implement such with target feature, and enable it for specific cpu. Another thing is that, if it is positive overall, why disable it by default?

[stefanp]

I only saw a minor nit with this patch overall.

To address the comment from the previous reviewer I don't feel we should enable this by default on a given CPU even if SPEC does not show significant degradations. This patch effectively tells the register allocator that it should never use the non-volatile CR registers if the option is given. If we make this the default we are effectively abandoning part of our hardware by telling the compiler never to use it. Anyway, that's my opinion on this...

llvm/test/CodeGen/PowerPC/ppc-disable-non-volatile-cr.ll

32

nit:
You can clean up the #2 from this line and the other two lines below.

[steven.zhang]

Well, this sounds like a tune option.I am fine with it. Just one concern on the implementation.

llvm/lib/Target/PowerPC/PPCRegisterInfo.h

25

This is not a good idea as you are polluting the namespace. If you really want to share information between compilation unit, you can declare local variable in sub target, and initialize it to the debug opt some where.

[nemanjai]

This needs to be a property of PPCFunctionInfo and set when the option is provided.

[NeHuang]

Addressed the comments by

• Add "DisableNonVolatileCR" into "PPCFunctionInfo" as a new property, add the functions to set and check it.
• Set the new member in the constructor based on the value of the option.
• Clean up redundant "#2" in the test case.

@nemanjai @stefanp @steven.zhang

[Yi-Hong.Lyu]

llvm/lib/Target/PowerPC/PPCRegisterInfo.td

369

Why do you NOT introduce a temp variable in the former condition but introduce one in the later one? I would suggest choose one style and use that uniformly.

378

Same as above.

[nemanjai]

The remaining comments are stylistic nits that can be addressed on the commit. Assuming that those will be fixed, LGTM.

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp

18 ↗

(On Diff #236438)

"Disable the use of non-volatile CR register fields."

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h

68 ↗

(On Diff #236438)

Line too long?

181 ↗

(On Diff #236438)

The naming is odd here - you use No for the setter and Disabled for the getter. Please name them consistently. Perhaps setDisableNonVolatileCR() and isNonVolatileCRDisabled().

llvm/lib/Target/PowerPC/PPCRegisterInfo.td

366

Please keep to 80 columns in .td files as well.

369

+1

[amyk]

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h

68 ↗

(On Diff #236438)

s/Inidcates/Indicates

Also, end with a period.

llvm/lib/Target/PowerPC/PPCRegisterInfo.h

 #include "MCTargetDesc/PPCMCTargetDesc.h"
 #include "llvm/ADT/DenseMap.h"
 
 #define GET_REGINFO_HEADER
 #include "PPCGenRegisterInfo.inc"
 
 namespace llvm {
 class PPCTargetMachine;
+extern cl::opt<bool> DisableNonVolatileCR;
 
 inline static unsigned getCRFromCRBit(unsigned SrcReg) {
   unsigned Reg = 0;
   if (SrcReg == PPC::CR0LT || SrcReg == PPC::CR0GT ||
       SrcReg == PPC::CR0EQ || SrcReg == PPC::CR0UN)
     Reg = PPC::CR0;
   else if (SrcReg == PPC::CR1LT || SrcReg == PPC::CR1GT ||
            SrcReg == PPC::CR1EQ || SrcReg == PPC::CR1UN)

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp

StackPtrConst("ppc-stack-ptr-caller-preserved",
                 cl::init(true), cl::Hidden);
 
 static cl::opt<unsigned>
 MaxCRBitSpillDist("ppc-max-crbit-spill-dist",
                   cl::desc("Maximum search distance for definition of CR bit "
                            "spill on ppc"),
                   cl::Hidden, cl::init(100));
 
+cl::opt<bool>
+llvm::DisableNonVolatileCR("ppc-disable-non-volatile-cr",
+                           cl::desc("Disable non-volatile CR/CR bit"),
+                           cl::init(false), cl::Hidden);
+
 static unsigned offsetMinAlignForOpcode(unsigned OpC);
 
 PPCRegisterInfo::PPCRegisterInfo(const PPCTargetMachine &TM)
   : PPCGenRegisterInfo(TM.isPPC64() ? PPC::LR8 : PPC::LR,
                        TM.isPPC64() ? 0 : 1,
                        TM.isPPC64() ? 0 : 1),
     TM(TM) {
   ImmToIdxMap[PPC::LD]   = PPC::LDX;    ImmToIdxMap[PPC::STD]  = PPC::STDX;

llvm/lib/Target/PowerPC/PPCRegisterInfo.td

(add CR2LT, CR2GT, CR2EQ, CR2UN,
        CR3LT, CR3GT, CR3EQ, CR3UN,
        CR4LT, CR4GT, CR4EQ, CR4UN,
        CR5LT, CR5GT, CR5EQ, CR5UN,
        CR6LT, CR6GT, CR6EQ, CR6UN,
        CR7LT, CR7GT, CR7EQ, CR7UN,
        CR1LT, CR1GT, CR1EQ, CR1UN,
        CR0LT, CR0GT, CR0EQ, CR0UN)> {
   let Size = 32;
+  let AltOrders = [(sub CRBITRC, CR2LT, CR2GT, CR2EQ, CR2UN, CR3LT, CR3GT, CR3EQ, CR3UN, CR4LT, CR4GT, CR4EQ, CR4UN)];
+  let AltOrderSelect = [{
+    return MF.getSubtarget<PPCSubtarget>().isELFv2ABI() && DisableNonVolatileCR;
+  }];
 }
 
-def CRRC : RegisterClass<"PPC", [i32], 32, (add CR0, CR1, CR5, CR6,
-                                                CR7, CR2, CR3, CR4)>;
-
+def CRRC : RegisterClass<"PPC", [i32], 32,
+  (add CR0, CR1, CR5, CR6,
+       CR7, CR2, CR3, CR4)> {
+  let AltOrders = [(sub CRRC, CR2, CR3, CR4)];
+  let AltOrderSelect = [{
+    return MF.getSubtarget<PPCSubtarget>().isELFv2ABI() && DisableNonVolatileCR;
+  }];
+}
 // The CTR registers are not allocatable because they're used by the
 // decrement-and-branch instructions, and thus need to stay live across
 // multiple basic blocks.
 def CTRRC : RegisterClass<"PPC", [i32], 32, (add CTR)> {
   let isAllocatable = 0;
 }
 def CTRRC8 : RegisterClass<"PPC", [i64], 64, (add CTR8)> {
   let isAllocatable = 0;
 }
 
 def VRSAVERC : RegisterClass<"PPC", [i32], 32, (add VRSAVE)>;
 def CARRYRC : RegisterClass<"PPC", [i32], 32, (add CARRY, XER)> {
   let CopyCost = -1;
 }
 

llvm/test/CodeGen/PowerPC/ppc-disable-non-volatile-cr.ll

+; Note: Test option to disable use of non-volatile CR to avoid CR spilling in prologue.
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu -ppc-disable-non-volatile-cr\
+; RUN:     -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr < %s | FileCheck --check-prefix=CHECK-DISABLE %s
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu\
+; RUN:     -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr < %s | FileCheck --check-prefix=CHECK-ENABLE %s
+
+; Function Attrs: nounwind
+define dso_local signext i32 @DisableNonVolatileCR(i32 signext %a, i32 signext %b) {
+; CHECK-DISABLE-LABEL: DisableNonVolatileCR:
+; CHECK-DISABLE:       # %bb.0: # %entry
+; CHECK-DISABLE-NOT:    mfocrf [[REG1:r[0-9]+]]
+; CHECK-DISABLE-NOT:    stw [[REG1]]
+; CHECK-DISABLE:        stdu r1
+; CHECK-DISABLE-DAG:    mfocrf [[REG2:r[0-9]+]]
+; CHECK-DISABLE-DAG:    stw [[REG2]]
+; CHECK-DISABLE:        # %bb.1: # %if.then
+;
+; CHECK-ENABLE-LABEL: DisableNonVolatileCR:
+; CHECK-ENABLE:       # %bb.0: # %entry
+; CHECK-ENABLE-DAG:    mfocrf [[REG1:r[0-9]+]]
+; CHECK-ENABLE-DAG:    stw [[REG1]]
+; CHECK-ENABLE:        stdu r1
+; CHECK-ENABLE-NOT:    mfocrf [[REG2:r[0-9]+]]
+; CHECK-ENABLE-NOT:    stw [[REG2]]
+; CHECK-ENABLE:        # %bb.1: # %if.then
+
+entry:
+  %cmp = icmp slt i32 %a, %b
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:                                          ; preds = %entry
+  tail call void bitcast (void (...)* @fa to void ()*)() #2
+  br label %if.end
+
+if.else:                                          ; preds = %entry
+  tail call void bitcast (void (...)* @fb to void ()*)() #2
+  br label %if.end
+
+if.end:                                           ; preds = %if.else, %if.then
+  %conv = zext i1 %cmp to i32
+  %call = tail call signext i32 @callee(i32 signext %conv) #2
+  ret i32 %call
+}
+
+declare void @fa(...)
+declare void @fb(...)
+declare signext i32 @callee(i32 signext)