[RISCV] Don't getDebugLoc for the end node of MBB iterator

[StephenFan]

Summary

Because of shrink wrapping, the block to insert epilog may don't have
instructions (Only debug instructions). And the position to insert may
point to MBB.end() that don't have a DebugLoc. This patch fix this
problem.

The test program was copied from the issue:https://github.com/llvm/llvm-project/issues/53662

[StephenFan]

ping

[jmorse]

Drive by review from a debug-info perspective -- I'm not a RISC-V person so don't feel confident enough to approve this, but I can see how the removed code will crash in the presence of a block only containing debug instructions, and that the new code will not crash. The fix looks correct in that sense, but someone else should confirm that it's not going to change codegen.

On the test front, IMO this is much better written as a MIR test that only runs prologepilog, where it will be resistant to optimisations, and the input to prologepilog will be very clear. It will also allow you to test that the correct (possibly empty) DebugLoc is assigned to instructions by the relevant function.

llvm/test/CodeGen/RISCV/xform_ah-min.ll

2 ↗

(On Diff #422503)

It's also best practice to check at least one line of output to ensure that what you want to happen, actually happened. That ensures the test doesn't spuriously pass in the future. An example would be if the IR Verifier was tightened to reject the debug-info in this file, it would load and compile this IR and pass the test, but the scenario you're testing wouldn't be covered.

Adding a CHECK line, for example stopping after prologepilog and checking the relevant block has the right contents, would prevent that.

41–42 ↗

(On Diff #422503)

Could you remove any unnecessary attributes, potentially all of them -- they're a future maintenance burden we can avoid now.

[StephenFan]

ChangeLog:

1. Rewrite .ll test as MIR test
2. Add '-run-pass=prologepilog' argument to test RUN line
3. Add a CHECK line.

[luismarques]

The fix looks fine.

Can you please clean up the Clang cruft from the MIR test file? And give it a meaningful name?
Shouldn't we also check the result of the pass?

If it helps, here's an automatically reduced test file, for reference:

|
  %struct.a = type { i32 }
  define i32 @b(%struct.a* %c) {
    %ab2 = getelementptr inbounds %struct.a, %struct.a* %c, i64 0, i32 0
    %1 = load i32, i32* %ab2, !tbaa !1
    br label %e.epilog
  e.epilog:
    %a.0.g = phi i32 [ 1, %e.epilog]
    br label %cleanup
  cleanup:
    %f.0 = phi i32 [ %a.0.g, %e.epilog ]
    ret i32 %f.0}
  !1 = !{}

[StephenFan]

ChangeLog:

1. rename test file to pr53662.mir
2. reduce pr53662.mir test file

[luismarques]

LGTM.

[StephenFan]

In D123679#3461601, @luismarques wrote:

The fix looks fine.

Can you please clean up the Clang cruft from the MIR test file? And give it a meaningful name?
Shouldn't we also check the result of the pass?

If it helps, here's an automatically reduced test file, for reference:

|
  %struct.a = type { i32 }
  define i32 @b(%struct.a* %c) {
    %ab2 = getelementptr inbounds %struct.a, %struct.a* %c, i64 0, i32 0
    %1 = load i32, i32* %ab2, !tbaa !1
    br label %e.epilog
  e.epilog:
    %a.0.g = phi i32 [ 1, %e.epilog]
    br label %cleanup
  cleanup:
    %f.0 = phi i32 [ %a.0.g, %e.epilog ]
    ret i32 %f.0}
  !1 = !{}

Oh, I forget to thank @luismarques for giving me the reduced test file, and the updated test file is inspired by it. Thanks, Luis!
But I want to remind you may be your automatically reduced tool is out-of-date, since the reduced test file you give me seems invalid.

[jrtc27]

Use update_mir_test_checks.py, don’t hand-write specific check lines

[StephenFan]

ChangeLoc:

Use update_mir_test_checks.py to autogenerate check lines

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

void RISCVFrameLowering::emitEpilogue(MachineFunction &MF,
   if (MF.getFunction().getCallingConv() == CallingConv::GHC)
     return;
 
   // Get the insert location for the epilogue. If there were no terminators in
   // the block, get the last instruction.
   MachineBasicBlock::iterator MBBI = MBB.end();
   DebugLoc DL;
   if (!MBB.empty()) {
-    MBBI = MBB.getFirstTerminator();
-    if (MBBI == MBB.end())
-      MBBI = MBB.getLastNonDebugInstr();
+    if (MBBI != MBB.end())
       DL = MBBI->getDebugLoc();
 
-    // If this is not a terminator, the actual insert location should be after the
-    // last instruction.
-    if (!MBBI->isTerminator())
-      MBBI = std::next(MBBI);
+    MBBI = MBB.getFirstTerminator();
 
     // If callee-saved registers are saved via libcall, place stack adjustment
     // before this call.
     while (MBBI != MBB.begin() &&
            std::prev(MBBI)->getFlag(MachineInstr::FrameDestroy))
       --MBBI;
   }
 

llvm/test/CodeGen/RISCV/pr53662.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=riscv64 -run-pass=prologepilog -o - %s | FileCheck %s
+--- |
+  define void @b() {
+  entry:
+    unreachable
+  }
+
+...
+---
+name:            b
+frameInfo:
+  savePoint:       '%bb.0'
+  restorePoint:    '%bb.1'
+body:             |
+  ; CHECK-LABEL: name: b
+  ; CHECK: bb.0:
+  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   PseudoBR %bb.1
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.1:
+  ; CHECK-NEXT:   successors: %bb.2(0x80000000)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   DBG_VALUE $noreg
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.2:
+  ; CHECK-NEXT:   PseudoRET implicit killed $x10
+  bb.0 :
+    PseudoBR %bb.1
+
+  bb.1:
+    DBG_VALUE $noreg
+
+  bb.2:
+    PseudoRET implicit killed $x10
+
+...