Check for all frame instructions in finalize isel.

[JonPsson1]

Check for all frame instructions in finalize isel, not just for the frame setup opcode. This has proved necessary as reported on
#78001 (comment).

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

[llvmbot]

@llvm/pr-subscribers-backend-systemz

Author: Jonas Paulsson (JonPsson1)

Changes

Check for all frame instructions in finalize isel, not just for the frame setup opcode. This has proved necessary as reported on
#78001 (comment).

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, &lt;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...&gt;, implicit-def dead $r14d, implicit-def dead $cc, implicit $fpc
  ADJCALLSTACKUP 0, 0

=&gt; 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, &lt;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...&gt;, 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 --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)

[nathanchance]

Thanks, this fixes that reported issue and the whole configuration builds fine for me with this patch. It'll get more intensive testing later today but I will only report back if something breaks.

[arsenm]

The change itself is fine but your description of the problem sounds like there's an iteration bug where the finalizeisel isn't resuming after a block split in the correct point. Is the SystemZ implementation of EmitInstrWithCustomInserter splitting the block incorrectly, or returning the wrong MBB pointer?

[JonPsson1]

The change itself is fine but your description of the problem sounds like there's an iteration bug where the finalizeisel isn't resuming after a block split in the correct point. Is the SystemZ implementation of EmitInstrWithCustomInserter splitting the block incorrectly, or returning the wrong MBB pointer?

The SystemZ EmitInstrWithCustomInserter is scanning forward in MBB in order to handle multiple selects at a time. It however stops on any instruction that clobbers CC (or another pseudo), which would include the call instruction in this case. The logical fix here might be to also stop an a frame setup instruction, but as the isel finalization continues with the new block where the call is, it will also reach the frame destroy instruction. So I think this looks ok.

[JonPsson1]

I agree it is slightly troublesome that the code in the backend can return any new MBB while making assumptions about which instructions to skip, and also by that have the FinalizeISelPass skip them. This was trivial as long as FinalizeISel only was interested in pseudos with custom insertion. As this changed, the backends assumption also had to change. In this case the SystemZ backend will be ok as long as the frame destroy opcode is also checked.

Maybe EmitInstrWithCustomInserter() should always return an iterator from where FinalizeISel should continue? That way the backend could scan forward a bit and skip things its unaware of while still returning an iterator so that this range is checked by FinalizeISel still.

Alternatively, maybe a comment something like this:

diff --git a/llvm/lib/CodeGen/FinalizeISel.cpp b/llvm/lib/CodeGen/FinalizeISel.cpp
index bf967eac22f1..9b920f6eaf82 100644
--- a/llvm/lib/CodeGen/FinalizeISel.cpp
+++ b/llvm/lib/CodeGen/FinalizeISel.cpp
@@ -67,6 +67,13 @@ bool FinalizeISel::runOnMachineFunction(MachineFunction &MF) {
         Changed = true;
         MachineBasicBlock *NewMBB = TLI->EmitInstrWithCustomInserter(MI, MBB);
         // The expansion may involve new basic blocks.
+
+        // NB! The target may skip past instructions while assuming they are
+        // not of interest to this pass, for example when SystemZ combines
+        // multiple selects into a single branch sequence (emitSelect). So if
+        // some new type of instruction becomes important here, it must be
+        // accompanied with some kind of assertion to make sure the backend
+        // does not mess this up.
         if (NewMBB != MBB) {

@arsenm @uweigand