[RISCV] Set how many bytes load from or store to stack slot

[Jim]

Summary

Refer from: https://reviews.llvm.org/D44782

After https://reviews.llvm.org/D130302, LW+SEXT.B can be folded into LB
as partially reload stack slot. This gains incorrect optimization result
from StackSlotColoring without given the number of bytes exactly load
from stack. LB+SW are mis-interpreted as fully reload/restore from stack
slot without the sign-extension. SW would be considered as a redundant store.

The testcase is copied from llvm/test/CodeGen/X86/pr30821.mir.

[craig.topper]

LGTM

[jrtc27]

llvm/test/CodeGen/RISCV/stack-slot-coloring.mir

1

Can we update_mir_test_checks.py this, precommit and see the diff in this patch?

[kito-cheng]

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

82

We might need to init MemByte to 0?

/// Optional extension of isLoadFromStackSlot that returns the number of
/// bytes loaded from the stack. This must be implemented if a backend
/// supports partial stack slot spills/loads to further disambiguate
/// what the load does.
virtual unsigned isLoadFromStackSlot(const MachineInstr &MI, 
                                     int &FrameIndex,
                                     unsigned &MemBytes) const {
  MemBytes = 0; 
  return isLoadFromStackSlot(MI, FrameIndex);
}

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

82

Do the callers care in the cases this function returns 0? Looks like MemBytes will be explicitly set on any path that returns a non-zero value.

[Jim]

Add pre-commit test case

[Jim]

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

82

It seems caller doesn't care MemBytes if the return value is zero (means it's not a load from or store to stack slot instruction).

llvm/test/CodeGen/RISCV/stack-slot-coloring.mir

1

Add pre-commit test case

[kito-cheng]

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

82

I am OK without init that to 0

[craig.topper]

LGTM

[StephenFan]

This patch caused a code size regression in our ci tests https://github.com/dtcxzyw/llvm-ci/issues/8. And part of the extra codes it generates is as follows:

I think the root cause is this patch makes the original function

unsigned isLoadFromStackSlot(const MachineInstr &MI,
                               int &FrameIndex) const override;

no longer be overridden.

[craig.topper]

In D145471#4187128, @StephenFan wrote:

This patch caused a code size regression in our ci tests https://github.com/dtcxzyw/llvm-ci/issues/8. And part of the extra codes it generates is as follows:

I think the root cause is this patch makes the original function

unsigned isLoadFromStackSlot(const MachineInstr &MI,
                               int &FrameIndex) const override;

no longer be overridden.

I have restored the original signatures and mapped them to the new signatures the same way x86 does.

llvm/lib/Target/RISCV/RISCVInstrInfo.h

 class RISCVInstrInfo : public RISCVGenInstrInfo {
 
 public:
   explicit RISCVInstrInfo(RISCVSubtarget &STI);
 
   MCInst getNop() const override;
   const MCInstrDesc &getBrCond(RISCVCC::CondCode CC) const;
 
-  unsigned isLoadFromStackSlot(const MachineInstr &MI,
-                               int &FrameIndex) const override;
-  unsigned isStoreToStackSlot(const MachineInstr &MI,
-                              int &FrameIndex) const override;
+  unsigned isLoadFromStackSlot(const MachineInstr &MI, int &FrameIndex,
+                               unsigned &MemBytes) const override;
+  unsigned isStoreToStackSlot(const MachineInstr &MI, int &FrameIndex,
+                              unsigned &MemBytes) const override;
 
   void copyPhysReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
                    const DebugLoc &DL, MCRegister DstReg, MCRegister SrcReg,
                    bool KillSrc) const override;
 
   void storeRegToStackSlot(MachineBasicBlock &MBB,
                            MachineBasicBlock::iterator MBBI, Register SrcReg,
                            bool IsKill, int FrameIndex,

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

if (STI.hasStdExtCOrZca())
     return MCInstBuilder(RISCV::C_NOP);
   return MCInstBuilder(RISCV::ADDI)
       .addReg(RISCV::X0)
       .addReg(RISCV::X0)
       .addImm(0);
 }
 
 unsigned RISCVInstrInfo::isLoadFromStackSlot(const MachineInstr &MI,
-                                             int &FrameIndex) const {
+                                             int &FrameIndex,
+                                             unsigned &MemBytes) const {
   switch (MI.getOpcode()) {
   default:
     return 0;
   case RISCV::LB:
   case RISCV::LBU:
+    MemBytes = 1;
+    break;
   case RISCV::LH:
   case RISCV::LHU:
   case RISCV::FLH:
+    MemBytes = 2;
+    break;
   case RISCV::LW:
   case RISCV::FLW:
   case RISCV::LWU:
+    MemBytes = 4;
+    break;
   case RISCV::LD:
   case RISCV::FLD:
+    MemBytes = 8;
     break;
   }
 
   if (MI.getOperand(1).isFI() && MI.getOperand(2).isImm() &&
       MI.getOperand(2).getImm() == 0) {
     FrameIndex = MI.getOperand(1).getIndex();
     return MI.getOperand(0).getReg();
   }
 
   return 0;
 }
 
 unsigned RISCVInstrInfo::isStoreToStackSlot(const MachineInstr &MI,
-                                            int &FrameIndex) const {
+                                            int &FrameIndex,
+                                            unsigned &MemBytes) const {
   switch (MI.getOpcode()) {
   default:
     return 0;
   case RISCV::SB:
+    MemBytes = 1;
+    break;
   case RISCV::SH:
-  case RISCV::SW:
   case RISCV::FSH:
+    MemBytes = 2;
+    break;
+  case RISCV::SW:
   case RISCV::FSW:
+    MemBytes = 4;
+    break;
   case RISCV::SD:
   case RISCV::FSD:
+    MemBytes = 8;
     break;
   }
 
   if (MI.getOperand(1).isFI() && MI.getOperand(2).isImm() &&
       MI.getOperand(2).getImm() == 0) {
     FrameIndex = MI.getOperand(1).getIndex();
     return MI.getOperand(0).getReg();
   }

llvm/test/CodeGen/RISCV/stack-slot-coloring.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 # RUN: llc -march=riscv32 -run-pass=greedy,virtregrewriter,stack-slot-coloring %s -o - 2>&1 | FileCheck %s
 
 --- |
   define dso_local i32 @main() local_unnamed_addr {
   entry:
     %a = alloca i32, align 4
     ret i32 0
   }
bb.0.entry:
     ; CHECK-NEXT: $x22 = LW %stack.0.a, 0 :: (volatile dereferenceable load (s32) from %ir.a)
     ; CHECK-NEXT: $x23 = LW %stack.0.a, 0 :: (volatile dereferenceable load (s32) from %ir.a)
     ; CHECK-NEXT: $x24 = LW %stack.0.a, 0 :: (volatile dereferenceable load (s32) from %ir.a)
     ; CHECK-NEXT: $x25 = LW %stack.0.a, 0 :: (volatile dereferenceable load (s32) from %ir.a)
     ; CHECK-NEXT: $x26 = LW %stack.0.a, 0 :: (volatile dereferenceable load (s32) from %ir.a)
     ; CHECK-NEXT: $x27 = LW %stack.0.a, 0 :: (volatile dereferenceable load (s32) from %ir.a)
     ; CHECK-NEXT: renamable $x1 = LW %stack.0.a, 0 :: (volatile dereferenceable load (s32) from %ir.a)
     ; CHECK-NEXT: SW killed renamable $x1, %stack.1, 0 :: (store (s32) into %stack.1)
+    ; CHECK-NEXT: renamable $x1 = LB %stack.0.a, 0 :: (volatile dereferenceable load (s8) from %ir.a)
+    ; CHECK-NEXT: SW killed renamable $x1, %stack.0.a, 0 :: (volatile store (s32) into %ir.a)
     ; CHECK-NEXT: renamable $x1 = LW %stack.1, 0 :: (load (s32) from %stack.1)
     ; CHECK-NEXT: SW killed renamable $x1, %stack.0.a, 0 :: (volatile store (s32) into %ir.a)
     ; CHECK-NEXT: renamable $x1 = LW %stack.0.a, 0 :: (volatile dereferenceable load (s32) from %ir.a)
     ; CHECK-NEXT: SW killed renamable $x1, %stack.1, 0 :: (store (s32) into %stack.1)
+    ; CHECK-NEXT: renamable $x1 = LB %stack.0.a, 0 :: (volatile dereferenceable load (s8) from %ir.a)
+    ; CHECK-NEXT: SW killed renamable $x1, %stack.0.a, 0 :: (volatile store (s32) into %ir.a)
     ; CHECK-NEXT: renamable $x1 = LW %stack.1, 0 :: (load (s32) from %stack.1)
     ; CHECK-NEXT: SW killed renamable $x1, %stack.0.a, 0 :: (volatile store (s32) into %ir.a)
     ; CHECK-NEXT: SW $x10, %stack.0.a, 0 :: (volatile store (s32) into %ir.a)
     ; CHECK-NEXT: SW $x11, %stack.0.a, 0 :: (volatile store (s32) into %ir.a)
     ; CHECK-NEXT: SW $x12, %stack.0.a, 0 :: (volatile store (s32) into %ir.a)
     ; CHECK-NEXT: SW $x13, %stack.0.a, 0 :: (volatile store (s32) into %ir.a)
     ; CHECK-NEXT: SW $x14, %stack.0.a, 0 :: (volatile store (s32) into %ir.a)
     ; CHECK-NEXT: SW $x15, %stack.0.a, 0 :: (volatile store (s32) into %ir.a)