[llvm] Check for all frame instructions in finalize isel. (PR #85945)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-backend-systemz

Author: Jonas Paulsson (JonPsson1)

<details>
<summary>Changes</summary>

Check for all frame instructions in finalize isel, not just for the frame setup opcode. This has proved necessary as reported on 
https://github.com/llvm/llvm-project/pull/78001#issuecomment-2008173278.

The probelm seems to be related to the SystemZ handling of multiple select instructions where it scans multiple Select64 and skips the ADJCALLSTACKDOWN pseudo:

```
  %4:gr64bit = Select64 killed %3:gr64bit, killed %2:gr64bit, 14, 6, implicit $cc
  ADJCALLSTACKDOWN 0, 0
  %5:gr64bit = LGHI 1
  %6:gr64bit = LGHI 6
  %7:gr64bit = Select64 killed %6:gr64bit, killed %5:gr64bit, 14, 6, implicit $cc
  $r2d = COPY %4:gr64bit
  CallBRASL @<!-- -->input_report_abs, $r2d, <regmask $f8d $f9d $f10d $f11d $f12d $f13d $f14d $f15d $f8q $f9q $f12q $f13q $f8s $f9s $f10s $f11s $f12s $f13s $f14s $f15s $r6d $r7d $r8d $r9d $r10d $r11d $r12d $r13d $r14d $r15d $r6h $r7h $r8h and 22 more...>, implicit-def dead $r14d, implicit-def dead $cc, implicit $fpc
  ADJCALLSTACKUP 0, 0

=> SystemZTargetLowering::emitSelect()

  ADJCALLSTACKDOWN 0, 0
  %5:gr64bit = LGHI 1
  %6:gr64bit = LGHI 6
  BRC 14, 6, %bb.1, implicit $cc

bb.2.entry:
; predecessors: %bb.0
  successors: %bb.1(0x80000000); %bb.1(100.00%)

bb.1.entry:
; predecessors: %bb.0, %bb.2

  %4:gr64bit = PHI %3:gr64bit, %bb.0, %2:gr64bit, %bb.2
  %7:gr64bit = PHI %6:gr64bit, %bb.0, %5:gr64bit, %bb.2
  $r2d = COPY %4:gr64bit
  CallBRASL @<!-- -->input_report_abs, $r2d, <regmask $f8d $f9d $f10d $f11d $f12d $f13d $f14d $f15d $f8q $f9q $f12q $f13q $f8s $f9s $f10s $f11s $f12s $f13s $f14s $f15s $r6d $r7d $r8d $r9d $r10d $r11d $r12d $r13d $r14d $r15d $r6h $r7h $r8h and 22 more...>, implicit-def dead $r14d, implicit-def dead $cc, implicit $fpc
  ADJCALLSTACKUP 0, 0
```

FinalizeISel now continues with the new MBB (bb.1.entry), and so doesn't see the ADJCALLSTACKDOWN instruction and fails to set adjustsStack.

I think either the SystemZ backend could recognize the frame instructions and either stop the optimization of selects or set AdjustsStack, or the FinalizeISel pass could recognize also the frame destroy opcode. I have chosen the latter as I think this would be generally useful while resolving this particular case.

I also think it was a mistake to remove the assertion in PEI - we should rather have *both* the assertion and the verifier detecting this. Assertion reinstated.

@<!-- -->arsenm @<!-- -->uweigand @<!-- -->nathanchance 

---
Full diff: https://github.com/llvm/llvm-project/pull/85945.diff


3 Files Affected:

- (modified) llvm/lib/CodeGen/FinalizeISel.cpp (+1-2) 
- (modified) llvm/lib/CodeGen/PrologEpilogInserter.cpp (+2) 
- (added) llvm/test/CodeGen/SystemZ/frame-adjstack.ll (+16) 


``````````diff
diff --git a/llvm/lib/CodeGen/FinalizeISel.cpp b/llvm/lib/CodeGen/FinalizeISel.cpp
index 978355f8eb1bbf..bf967eac22f177 100644
--- a/llvm/lib/CodeGen/FinalizeISel.cpp
+++ b/llvm/lib/CodeGen/FinalizeISel.cpp
@@ -59,8 +59,7 @@ bool FinalizeISel::runOnMachineFunction(MachineFunction &MF) {
 
       // Set AdjustsStack to true if the instruction selector emits a stack
       // frame setup instruction or a stack aligning inlineasm.
-      if (MI.getOpcode() == TII->getCallFrameSetupOpcode() ||
-          MI.isStackAligningInlineAsm())
+      if (TII->isFrameInstr(MI) || MI.isStackAligningInlineAsm())
         MF.getFrameInfo().setAdjustsStack(true);
 
       // If MI is a pseudo, expand it.
diff --git a/llvm/lib/CodeGen/PrologEpilogInserter.cpp b/llvm/lib/CodeGen/PrologEpilogInserter.cpp
index c942b8a3e26880..eaf96ec5cbde8c 100644
--- a/llvm/lib/CodeGen/PrologEpilogInserter.cpp
+++ b/llvm/lib/CodeGen/PrologEpilogInserter.cpp
@@ -372,6 +372,8 @@ void PEI::calculateCallFrameInfo(MachineFunction &MF) {
   MFI.computeMaxCallFrameSize(MF, &FrameSDOps);
   assert(MFI.getMaxCallFrameSize() <= MaxCFSIn &&
          "Recomputing MaxCFS gave a larger value.");
+  assert((FrameSDOps.empty() || MF.getFrameInfo().adjustsStack()) &&
+         "AdjustsStack not set in presence of a frame pseudo instruction.");
 
   if (TFI->canSimplifyCallFramePseudos(MF)) {
     // If call frames are not being included as part of the stack frame, and
diff --git a/llvm/test/CodeGen/SystemZ/frame-adjstack.ll b/llvm/test/CodeGen/SystemZ/frame-adjstack.ll
new file mode 100644
index 00000000000000..c072a596c7c29c
--- /dev/null
+++ b/llvm/test/CodeGen/SystemZ/frame-adjstack.ll
@@ -0,0 +1,16 @@
+; RUN: llc < %s -mtriple=s390x-linux-gnu | FileCheck %s
+;
+; Test that inserting a new MBB near a call during finalize isel custom
+; insertion does not cause all frame instructions to be missed. That would
+; result in a missing to set the AdjustsStack flag.
+
+; CHECK-LABEL: fun
+define void @fun(i1 %cc) {
+  %sel = select i1 %cc, i32 5, i32 0
+  tail call void @input_report_abs(i32 %sel)
+  %sel2 = select i1 %cc, i32 6, i32 1
+  tail call void @input_report_abs(i32 %sel2)
+  ret void
+}
+
+declare void @input_report_abs(i32)

``````````

</details>


https://github.com/llvm/llvm-project/pull/85945