[RISCV] Fix wrong register rename for store value during make-compressible optimization

[kito-cheng]

Summary

Current implementation will rename both register in store instructions if
we store base address into memory with same base register, it's OK if
the offset is 0, however that is wrong transform if offset isn't 0, give
a smalle example here:

sd a0, 808(a0)

We should not transform into:

addi a2, a0, 768
sd a2, 40(a2)

That should just rename base address like this:

addi a2, a0, 768
sd a0, 40(a2)

[craig.topper]

llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp

105

Methods name start with lower case.

Is this ever called?

[kito-cheng]

Changes:

• Rename Incompressible -> incompressible
• Use incompressible function to check it's incompressible or not.
• Use const ref if possible for CompressibleInfo

[kito-cheng]

llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp

105

Use CI.incompressible() instead of !RegImm.Reg && RegImm.Imm == 0 now

[kito-cheng]

Changes:

• Less invasive way to fix this issue.

[asb]

LGTM, though see suggestion about slightly more detailed comment.

llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp

304

This is a bit subtle, but easy to understand when spelled out so I'd be tempted to write a slightly more in-depth comment. e.g.:

Perhaps "Skip the first (value) operand to a store instruction (except if the store offset is zero) in order to avoid an incorrect transformation. e.g. sd a0, 808(a0) to addi a2, a0, 768; sd a2, 40(a2)"

llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp

struct RISCVMakeCompressibleOpt : public MachineFunctionPass {
   bool runOnMachineFunction(MachineFunction &Fn) override;
 
   RISCVMakeCompressibleOpt() : MachineFunctionPass(ID) {
     initializeRISCVMakeCompressibleOptPass(*PassRegistry::getPassRegistry());
   }
 
   StringRef getPassName() const override { return RISCV_COMPRESS_INSTRS_NAME; }
 };
+
+// Holds information about registers that need to be renamed to make
+// instructions compressible.
+struct CompressibleInfo {
+  // Register for base register or SrcDest register
+  Register Reg = RISCV::NoRegister;
+  // Offset of base address
+  int64_t Offset = 0;
+  // Target is base address register or SrcDest register operand in the
+  // instruction.
+  bool IsBase = false;
+  // Indicate there is no compressible chance.
+  bool Incompressible() const {
+    return Reg == RISCV::NoRegister && Offset == 0;
+  }
+};
+
 } // namespace
 
 char RISCVMakeCompressibleOpt::ID = 0;
 INITIALIZE_PASS(RISCVMakeCompressibleOpt, "riscv-make-compressible",
                 RISCV_COMPRESS_INSTRS_NAME, false, false)
 
 // Return log2(widthInBytes) of load/store done by Opcode.
 static unsigned log2LdstWidth(unsigned Opcode) {
static bool isCompressibleStore(const MachineInstr &MI) {
   const unsigned Opcode = MI.getOpcode();
 
   return Opcode == RISCV::SW || (!STI.is64Bit() && Opcode == RISCV::FSW) ||
          Opcode == RISCV::SD || Opcode == RISCV::FSD;
 }
 
 // Find a single register and/or large offset which, if compressible, would
 // allow the given instruction to be compressed.
-//
-// Possible return values:
-//
-//   {Reg, 0}               - Uncompressed Reg needs replacing with a compressed
-//                            register.
-//   {Reg, N}               - Reg needs replacing with a compressed register and
-//                            N needs adding to the new register. (Reg may be
-//                            compressed or uncompressed).
-//   {RISCV::NoRegister, 0} - No suitable optimization found for this
-//   instruction.
-static RegImmPair getRegImmPairPreventingCompression(const MachineInstr &MI) {
+static CompressibleInfo getPreventingCompression(const MachineInstr &MI) {
   const unsigned Opcode = MI.getOpcode();
 
   if (isCompressibleLoad(MI) || isCompressibleStore(MI)) {
     const MachineOperand &MOImm = MI.getOperand(2);
     if (!MOImm.isImm())
-      return RegImmPair(RISCV::NoRegister, 0);
+      return CompressibleInfo();
 
     int64_t Offset = MOImm.getImm();
     int64_t NewBaseAdjust = getBaseAdjustForCompression(Offset, Opcode);
     Register Base = MI.getOperand(1).getReg();
 
     // Memory accesses via the stack pointer do not have a requirement for
     // either of the registers to be compressible and can take a larger offset.
     if (RISCV::SPRegClass.contains(Base)) {
       if (!compressibleSPOffset(Offset, Opcode) && NewBaseAdjust)
-        return RegImmPair(Base, NewBaseAdjust);
+        return CompressibleInfo({Base, NewBaseAdjust, /*IsBase*/ true});
     } else {
       Register SrcDest = MI.getOperand(0).getReg();
       bool SrcDestCompressed = isCompressedReg(SrcDest);
       bool BaseCompressed = isCompressedReg(Base);
 
       // If only Base and/or offset prevent compression, then return Base and
       // any adjustment required to make the offset compressible.
       if ((!BaseCompressed || NewBaseAdjust) && SrcDestCompressed)
-        return RegImmPair(Base, NewBaseAdjust);
+        return CompressibleInfo({Base, NewBaseAdjust, /*IsBase*/ true});
 
       // For loads, we can only change the base register since dest is defined
       // rather than used.
       //
       // For stores, we can change SrcDest (and Base if SrcDest == Base) but
       // cannot resolve an uncompressible offset in this case.
       if (isCompressibleStore(MI)) {
         if (!SrcDestCompressed && (BaseCompressed || SrcDest == Base) &&
             !NewBaseAdjust)
-          return RegImmPair(SrcDest, NewBaseAdjust);
+          return CompressibleInfo({SrcDest, NewBaseAdjust, /*IsBase*/ false});
       }
     }
   }
-  return RegImmPair(RISCV::NoRegister, 0);
+  return CompressibleInfo();
 }
 
 // Check all uses after FirstMI of the given register, keeping a vector of
 // instructions that would be compressible if the given register (and offset if
 // applicable) were compressible.
 //
 // If there are enough uses for this optimization to improve code size and a
 // compressed register is available, return that compressed register.
 static Register analyzeCompressibleUses(MachineInstr &FirstMI,
-                                        RegImmPair RegImm,
+                                        CompressibleInfo CI,
                                         SmallVectorImpl<MachineInstr *> &MIs) {
   MachineBasicBlock &MBB = *FirstMI.getParent();
   const TargetRegisterInfo *TRI =
       MBB.getParent()->getSubtarget().getRegisterInfo();
 
   RegScavenger RS;
   RS.enterBasicBlock(MBB);
 
   for (MachineBasicBlock::instr_iterator I = FirstMI.getIterator(),
                                          E = MBB.instr_end();
        I != E; ++I) {
     MachineInstr &MI = *I;
 
     // Determine if this is an instruction which would benefit from using the
     // new register.
-    RegImmPair CandidateRegImm = getRegImmPairPreventingCompression(MI);
-    if (CandidateRegImm.Reg == RegImm.Reg &&
-        CandidateRegImm.Imm == RegImm.Imm) {
+    CompressibleInfo Candidate = getPreventingCompression(MI);
+    if (Candidate.Reg == CI.Reg && Candidate.Offset == CI.Offset) {
       // Advance tracking since the value in the new register must be live for
       // this instruction too.
       RS.forward(I);
 
       MIs.push_back(&MI);
     }
 
-    // If RegImm.Reg is modified by this instruction, then we cannot optimize
+    // If Reg is modified by this instruction, then we cannot optimize
     // past this instruction. If the register is already compressed, then it may
     // possible to optimize a large offset in the current instruction - this
     // will have been detected by the preceeding call to
     // getRegImmPairPreventingCompression.
-    if (MI.modifiesRegister(RegImm.Reg, TRI))
+    if (MI.modifiesRegister(CI.Reg, TRI))
       break;
   }
 
   // Adjusting the base costs one new uncompressed addi and therefore three uses
   // are required for a code size reduction. If no base adjustment is required,
   // then copying the register costs one new c.mv (or c.li Rd, 0 for "copying"
   // the zero register) and therefore two uses are required for a code size
   // reduction.
-  if (MIs.size() < 2 || (RegImm.Imm != 0 && MIs.size() < 3))
+  if (MIs.size() < 2 || (CI.Offset != 0 && MIs.size() < 3))
     return RISCV::NoRegister;
 
   // Find a compressible register which will be available from the first
   // instruction we care about to the last.
   const TargetRegisterClass *RCToScavenge;
 
   // Work out the compressed register class from which to scavenge.
-  if (RISCV::GPRRegClass.contains(RegImm.Reg))
+  if (RISCV::GPRRegClass.contains(CI.Reg))
     RCToScavenge = &RISCV::GPRCRegClass;
-  else if (RISCV::FPR32RegClass.contains(RegImm.Reg))
+  else if (RISCV::FPR32RegClass.contains(CI.Reg))
     RCToScavenge = &RISCV::FPR32CRegClass;
-  else if (RISCV::FPR64RegClass.contains(RegImm.Reg))
+  else if (RISCV::FPR64RegClass.contains(CI.Reg))
     RCToScavenge = &RISCV::FPR64CRegClass;
   else
     return RISCV::NoRegister;
 
   return RS.scavengeRegisterBackwards(*RCToScavenge, FirstMI.getIterator(),
                                       /*RestoreAfter=*/false, /*SPAdj=*/0,
                                       /*AllowSpill=*/false);
 }
 
 // Update uses of the old register in the given instruction to the new register.
-static void updateOperands(MachineInstr &MI, RegImmPair OldRegImm,
+static void updateOperands(MachineInstr &MI, CompressibleInfo CI,
                            Register NewReg) {
   unsigned Opcode = MI.getOpcode();
 
   // If this pass is extended to support more instructions, the check for
   // definedness may need to be strengthened.
   assert((isCompressibleLoad(MI) || isCompressibleStore(MI)) &&
          "Unsupported instruction for this optimization.");
 
+  // Skip first operand for store instruction, it's operand for store value.
+  int SkipN = isCompressibleStore(MI) && CI.IsBase && (CI.Offset != 0) ? 1 : 0;
+
   // Update registers
-  for (MachineOperand &MO : MI.operands())
-    if (MO.isReg() && MO.getReg() == OldRegImm.Reg) {
+  for (MachineOperand &MO : drop_begin(MI.operands(), SkipN))
+    if (MO.isReg() && MO.getReg() == CI.Reg) {
       // Do not update operands that define the old register.
       //
       // The new register was scavenged for the range of instructions that are
       // being updated, therefore it should not be defined within this range
       // except possibly in the final instruction.
       if (MO.isDef()) {
         assert(isCompressibleLoad(MI));
         continue;
bool RISCVMakeCompressibleOpt::runOnMachineFunction(MachineFunction &Fn) {
   if (!STI.hasStdExtC())
     return false;
 
   for (MachineBasicBlock &MBB : Fn) {
     LLVM_DEBUG(dbgs() << "MBB: " << MBB.getName() << "\n");
     for (MachineInstr &MI : MBB) {
       // Determine if this instruction would otherwise be compressed if not for
       // an uncompressible register or offset.
-      RegImmPair RegImm = getRegImmPairPreventingCompression(MI);
-      if (!RegImm.Reg && RegImm.Imm == 0)
+      CompressibleInfo CI = getPreventingCompression(MI);
+      if (!CI.Reg && CI.Offset == 0)
         continue;
 
       // Determine if there is a set of instructions for which replacing this
       // register with a compressed register (and compressible offset if
       // applicable) is possible and will allow compression.
       SmallVector<MachineInstr *, 8> MIs;
-      Register NewReg = analyzeCompressibleUses(MI, RegImm, MIs);
+      Register NewReg = analyzeCompressibleUses(MI, CI, MIs);
       if (!NewReg)
         continue;
 
       // Create the appropriate copy and/or offset.
-      if (RISCV::GPRRegClass.contains(RegImm.Reg)) {
-        assert(isInt<12>(RegImm.Imm));
+      if (RISCV::GPRRegClass.contains(CI.Reg)) {
+        assert(isInt<12>(CI.Offset));
         BuildMI(MBB, MI, MI.getDebugLoc(), TII.get(RISCV::ADDI), NewReg)
-            .addReg(RegImm.Reg)
-            .addImm(RegImm.Imm);
+            .addReg(CI.Reg)
+            .addImm(CI.Offset);
       } else {
         // If we are looking at replacing an FPR register we don't expect to
         // have any offset. The only compressible FP instructions with an offset
         // are loads and stores, for which the offset applies to the GPR operand
         // not the FPR operand.
-        assert(RegImm.Imm == 0);
-        unsigned Opcode = RISCV::FPR32RegClass.contains(RegImm.Reg)
+        assert(CI.Offset == 0);
+        unsigned Opcode = RISCV::FPR32RegClass.contains(CI.Reg)
                               ? RISCV::FSGNJ_S
                               : RISCV::FSGNJ_D;
         BuildMI(MBB, MI, MI.getDebugLoc(), TII.get(Opcode), NewReg)
-            .addReg(RegImm.Reg)
-            .addReg(RegImm.Reg);
+            .addReg(CI.Reg)
+            .addReg(CI.Reg);
       }
 
       // Update the set of instructions to use the compressed register and
       // compressible offset instead. These instructions should now be
       // compressible.
-      // TODO: Update all uses if RegImm.Imm == 0? Not just those that are
+      // TODO: Update all uses if Offset == 0? Not just those that are
       // expected to become compressible.
       for (MachineInstr *UpdateMI : MIs)
-        updateOperands(*UpdateMI, RegImm, NewReg);
+        updateOperands(*UpdateMI, CI, NewReg);
     }
   }
   return true;
 }
 
 /// Returns an instance of the Make Compressible Optimization pass.
 FunctionPass *llvm::createRISCVMakeCompressibleOptPass() {
   return new RISCVMakeCompressibleOpt();
 }

llvm/test/CodeGen/RISCV/make-compressible-for-store-address.mir

bb.0.entry:
     liveins: $x10
 
     ; CHECK-LABEL: name: foo
     ; CHECK: liveins: $x10
     ; CHECK-NEXT: {{  $}}
     ; CHECK-NEXT: renamable $x11 = ADDI $x0, 1
     ; CHECK-NEXT: $x12 = ADDI $x10, 768
     ; CHECK-NEXT: SD killed renamable $x11, $x12, 32 :: (store (s64) into %ir.1)
-    ; CHECK-NEXT: SD $x12, $x12, 40 :: (store (s64) into %ir.2)
+    ; CHECK-NEXT: SD renamable $x10, $x12, 40 :: (store (s64) into %ir.2)
     ; CHECK-NEXT: renamable $x11 = ADDI $x0, 2
     ; CHECK-NEXT: SD killed renamable $x11, killed $x12, 48 :: (store (s64) into %ir.3)
     ; CHECK-NEXT: PseudoRET
     renamable $x11 = ADDI $x0, 1
     SD killed renamable $x11, renamable $x10, 800 :: (store (s64) into %ir.1)
     SD renamable $x10, renamable $x10, 808 :: (store (s64) into %ir.2)
     renamable $x11 = ADDI $x0, 2
     SD killed renamable $x11, killed renamable $x10, 816 :: (store (s64) into %ir.3)
     PseudoRET
 
 ...