Fix invalid Kill on callee save on RISC-V

[david2050]

Summary

A callee save may be live (specifically X1) on entry and so a spill should not mark it killed.

[david2050]

An example of incorrect code in baseline is below where the spill of X1 is marked killed but is live to the following SW.

Function Live Ins: $x1

bb.0.entry:
  liveins: $x1
  $x2 = frame-setup ADDI $x2, -32
  CFI_INSTRUCTION def_cfa_offset 32
  SD killed $x1, $x2, 24
  SD killed $x8, $x2, 16
  CFI_INSTRUCTION offset $x1, -8
  CFI_INSTRUCTION offset $x8, -16
  $x8 = frame-setup ADDI $x2, 32
  CFI_INSTRUCTION def_cfa $x8, 0
  SW killed renamable $x1, $x8, -20 :: (store 4 into %ir.a)

[craig.topper]

Can you add a test case? Do you have an IR example?

[jrtc27]

I believe:

declare void @callee(i8**)

define void @caller() nounwind {
  %1 = alloca i8*
  %2 = call i8* @llvm.returnaddress(i32 0)
  store i8* %2, i8** %1
  call void @callee(i8** %1)
  ret void
}

with -stop-after=prologepilog should work. Or perhaps better to take the MIR for that with -stop-before=prologepilog and run just prologepilog on it in the test.

[david2050]

Add test case, thanks @jrtc27

[david2050]

clang format

[craig.topper]

llvm/test/CodeGen/RISCV/live_sp.ll

1 ↗

(On Diff #377879)

@jrtc27 should we use update_mir_test_checks.py to get all of the CHECK lines?

2 ↗

(On Diff #377879)

Drop this comment point to the review. It will be in the commit log for the file if anyone needs it.

[david2050]

Change unit test to run just prologepilog phase and use update_mir_test_checks.py

[david2050]

Thanks Craig

[luismarques]

Seems fine now. Any other comments? @craig.topper @jrtc27

llvm/test/CodeGen/RISCV/live_sp.mir

4–8 ↗

(On Diff #377972)

Nit: remove this and add -march=riscv64 in RUN line instead?

[asb]

This looks good to me., thanks you! Very minor nits on the test file:

• I agree with Luis that modifying to use -march would be marginally better (you no longer need to worry if the datalayout string is the current standard RISC-V one for instance)
• Although a comment linking to this review thread doesn't make sense, it would probably be helpful to add a comment to note that the live X1 register isn't incorrectly marked as killed when being spilled.

[asb]

@luke957 your Herald rule seems to automatically add you as a blocking reviewer - could you please adjust it. It would probably be best to just have the rule add you as a subscriber.

[luke957]

So sorry for my bad herald script.

[david2050]

I made the two tweaks suggested but as it turns out I can't commit to github, can someone land this for me?

[asb]

Now committed - thanks again for the patch!

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

bool RISCVFrameLowering::spillCalleeSavedRegisters(
   }
 
   // Manually spill values not spilled by libcall.
   const auto &NonLibcallCSI = getNonLibcallCSI(*MF, CSI);
   for (auto &CS : NonLibcallCSI) {
     // Insert the spill to the stack frame.
     Register Reg = CS.getReg();
     const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(Reg);
-    TII.storeRegToStackSlot(MBB, MI, Reg, true, CS.getFrameIdx(), RC, TRI);
+    TII.storeRegToStackSlot(MBB, MI, Reg, !MBB.isLiveIn(Reg), CS.getFrameIdx(),
+                            RC, TRI);
   }
 
   return true;
 }
 
 bool RISCVFrameLowering::restoreCalleeSavedRegisters(
     MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
     MutableArrayRef<CalleeSavedInfo> CSI, const TargetRegisterInfo *TRI) const {

llvm/test/CodeGen/RISCV/live-sp.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -march=riscv64 -run-pass=prologepilog -simplify-mir  -o - %s | FileCheck %s
+# verify live-on-entry registers are not marked killed by spills
+--- |
+
+  declare void @vararg(i32 signext, ...)
+
+  define void @test1() {
+  entry:
+    %a = alloca i32, align 4
+    %0 = call i8* @llvm.returnaddress(i32 0)
+    %1 = ptrtoint i8* %0 to i64
+    %conv = trunc i64 %1 to i32
+    store i32 %conv, i32* %a, align 4
+    %2 = load i32, i32* %a, align 4
+    call void (i32, ...) @vararg(i32 signext 0, i32 signext %2)
+    ret void
+  }
+
+  ; Function Attrs: nofree nosync nounwind readnone willreturn
+  declare i8* @llvm.returnaddress(i32 immarg) #0
+
+  attributes #0 = { nofree nosync nounwind readnone willreturn }
+
+...
+---
+name:            test1
+alignment:       4
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+failedISel:      false
+tracksRegLiveness: true
+hasWinCFI:       false
+registers:       []
+liveins:
+  - { reg: '$x1', virtual-reg: '' }
+frameInfo:
+  isFrameAddressTaken: false
+  isReturnAddressTaken: true
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    4
+  adjustsStack:    false
+  hasCalls:        true
+  stackProtector:  ''
+  maxCallFrameSize: 4294967295
+  cvBytesOfCalleeSavedRegisters: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+  hasTailCall:     false
+  localFrameSize:  0
+  savePoint:       ''
+  restorePoint:    ''
+fixedStack:      []
+stack:
+  - { id: 0, name: a, type: default, offset: 0, size: 4, alignment: 4,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+callSites:       []
+debugValueSubstitutions: []
+constants:       []
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    liveins: $x1
+
+    ; CHECK-LABEL: name: test1
+    ; CHECK: liveins: $x1
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: $x2 = frame-setup ADDI $x2, -16
+    ; CHECK-NEXT: frame-setup CFI_INSTRUCTION def_cfa_offset 16
+    ; CHECK-NEXT: SD $x1, $x2, 8 :: (store (s64) into %stack.1)
+    ; CHECK-NEXT: frame-setup CFI_INSTRUCTION offset $x1, -8
+    ; CHECK-NEXT: SW renamable $x1, $x2, 4 :: (store (s32) into %ir.a)
+    ; CHECK-NEXT: renamable $x11 = ADDIW killed renamable $x1, 0
+    ; CHECK-NEXT: $x10 = COPY $x0
+    ; CHECK-NEXT: PseudoCALL target-flags(riscv-plt) @vararg, csr_ilp32_lp64, implicit-def dead $x1, implicit killed $x10, implicit $x11, implicit-def $x2
+    ; CHECK-NEXT: $x1 = LD $x2, 8 :: (load (s64) from %stack.1)
+    ; CHECK-NEXT: $x2 = frame-destroy ADDI $x2, 16
+    ; CHECK-NEXT: PseudoRET
+    SW renamable $x1, %stack.0.a, 0 :: (store (s32) into %ir.a)
+    renamable $x11 = ADDIW killed renamable $x1, 0
+    ADJCALLSTACKDOWN 0, 0, implicit-def dead $x2, implicit $x2
+    $x10 = COPY $x0
+    PseudoCALL target-flags(riscv-plt) @vararg, csr_ilp32_lp64, implicit-def dead $x1, implicit killed $x10, implicit $x11, implicit-def $x2
+    ADJCALLSTACKUP 0, 0, implicit-def dead $x2, implicit $x2
+    PseudoRET
+
+...