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

33

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 ↗

(On Diff #227803)

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.

380

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

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

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h

68

Line too long?

182

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

s/Inidcates/Indicates

Also, end with a period.

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h

class PPCFunctionInfo : public MachineFunctionInfo {
 
   /// Does this function spill using instructions with only r+r (not r+i)
   /// forms.
   bool HasNonRISpills = false;
 
   /// SpillsCR - Indicates whether CR is spilled in the current function.
   bool SpillsCR = false;
 
+  /// DisableNonVolatileCR - Indicates whether non-volatile CR fields would be
+  /// disabled.
+  bool DisableNonVolatileCR = false;
+
   /// Indicates whether VRSAVE is spilled in the current function.
   bool SpillsVRSAVE = false;
 
   /// LRStoreRequired - The bool indicates whether there is some explicit use of
   /// the LR/LR8 stack slot that is not obvious from scanning the code.  This
   /// requires that the code generator produce a store of LR to the stack on
   /// entry, even though LR may otherwise apparently not be used.
   bool LRStoreRequired = false;
class PPCFunctionInfo : public MachineFunctionInfo {
   /// copies
   bool IsSplitCSR = false;
 
   /// We keep track attributes for each live-in virtual registers
   /// to use SExt/ZExt flags in later optimization.
   std::vector<std::pair<unsigned, ISD::ArgFlagsTy>> LiveInAttrs;
 
 public:
-  explicit PPCFunctionInfo(MachineFunction &MF) : MF(MF) {}
+  explicit PPCFunctionInfo(MachineFunction &MF);
 
   int getFramePointerSaveIndex() const { return FramePointerSaveIndex; }
   void setFramePointerSaveIndex(int Idx) { FramePointerSaveIndex = Idx; }
 
   int getReturnAddrSaveIndex() const { return ReturnAddrSaveIndex; }
   void setReturnAddrSaveIndex(int idx) { ReturnAddrSaveIndex = idx; }
 
   int getBasePointerSaveIndex() const { return BasePointerSaveIndex; }
public:
   bool hasSpills() const   { return HasSpills; }
 
   void setHasNonRISpills()    { HasNonRISpills = true; }
   bool hasNonRISpills() const { return HasNonRISpills; }
 
   void setSpillsCR()       { SpillsCR = true; }
   bool isCRSpilled() const { return SpillsCR; }
 
+  void setDisableNonVolatileCR() { DisableNonVolatileCR = true; }
+  bool isNonVolatileCRDisabled() const { return DisableNonVolatileCR; }
+
   void setSpillsVRSAVE()       { SpillsVRSAVE = true; }
   bool isVRSAVESpilled() const { return SpillsVRSAVE; }
 
   void setLRStoreRequired() { LRStoreRequired = true; }
   bool isLRStoreRequired() const { return LRStoreRequired; }
 
   void setUsesTOCBasePtr()    { UsesTOCBasePtr = true; }
   bool usesTOCBasePtr() const { return UsesTOCBasePtr; }

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp

 //===-- PPCMachineFunctionInfo.cpp - Private data used for PowerPC --------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "PPCMachineFunctionInfo.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/MC/MCContext.h"
+#include "llvm/Support/CommandLine.h"
 
 using namespace llvm;
+static cl::opt<bool> PPCDisableNonVolatileCR(
+    "ppc-disable-non-volatile-cr",
+    cl::desc("Disable the use of non-volatile CR register fields"),
+    cl::init(false), cl::Hidden);
 
 void PPCFunctionInfo::anchor() {}
+PPCFunctionInfo::PPCFunctionInfo(MachineFunction &MF)
+    : DisableNonVolatileCR(PPCDisableNonVolatileCR), MF(MF) {}
 
 MCSymbol *PPCFunctionInfo::getPICOffsetSymbol() const {
   const DataLayout &DL = MF.getDataLayout();
   return MF.getContext().getOrCreateSymbol(Twine(DL.getPrivateGlobalPrefix()) +
                                            Twine(MF.getFunctionNumber()) +
                                            "$poff");
 }
 

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() &&
+           MF.getInfo<PPCFunctionInfo>()->isNonVolatileCRDisabled();
+  }];
 }
 
-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() &&
+           MF.getInfo<PPCFunctionInfo>()->isNonVolatileCRDisabled();
+  }];
+}
 // 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 ()*)()
+  br label %if.end
+
+if.else:                                          ; preds = %entry
+  tail call void bitcast (void (...)* @fb to void ()*)()
+  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)
+  ret i32 %call
+}
+
+declare void @fa(...)
+declare void @fb(...)
+declare signext i32 @callee(i32 signext)