[PATCH] D92803: [SystemZFrameLowering] Make sure R1 holding the backchain is not corrupted by probing

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa created this revision.
jonpa added a reviewer: uweigand.
Herald added a subscriber: hiraditya.
jonpa requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

For a function that saves the backchain while allocating stack space with probing (stack clash protection), R1D is used for both of these purposes. The probing modifies R1D in order to have a reference value for exiting the loop. In order to save the original value of R15D as the backchain after the probing-loop, the value decremented from R1D must be added back.

An additional improvement is to not copy R15 <https://reviews.llvm.org/source/zorg/> to R1 <https://reviews.llvm.org/diffusion/L/> in this case, since then it has already been done for the purpose of the backchain.

I am not sure if there is a better way, but this seems ok as long as adding it back is just a single instruction (would there be any other reg available?).

This was incidentally discovered as a big function with "backchain" and "probe-stack"="inline-asm" was discovered by csmith/machine-verifier to not have R1D live-in to DoneMBB where the backchain is saved by an STG. This was due to the curious fact that the StackAllocMI is erased *after* recomputeLiveIns(*DoneMBB) is called, and since PROBED_STACKALLOC is marked only as defining R1D, it was not live-in.

I started by adding R1D as live into DoneMBB in SystemZFrameLowering::inlineStackProbe(), but then realized that the problem seems to be even worse: R1D is used in the probing loop after first being decremented with LoopAlloc. That means that the backchain value is in fact not the incoming R15D anymore in this case.


https://reviews.llvm.org/D92803

Files:
  llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
  llvm/test/CodeGen/SystemZ/stack-clash-protection.ll


Index: llvm/test/CodeGen/SystemZ/stack-clash-protection.ll
===================================================================
--- llvm/test/CodeGen/SystemZ/stack-clash-protection.ll
+++ llvm/test/CodeGen/SystemZ/stack-clash-protection.ll
@@ -237,6 +237,37 @@
   ret i32 %c
 }
 
+define void @fun9() #0 "backchain" {
+; CHECK-LABEL: fun9:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    lgr %r1, %r15
+; CHECK-NEXT:    .cfi_def_cfa_register %r1
+; CHECK-NEXT:    aghi %r1, -28672
+; CHECK-NEXT:    .cfi_def_cfa_offset 28832
+; CHECK-NEXT:  .LBB9_1: # %entry
+; CHECK-NEXT:    # =>This Inner Loop Header: Depth=1
+; CHECK-NEXT:    aghi %r15, -4096
+; CHECK-NEXT:    cg %r0, 4088(%r15)
+; CHECK-NEXT:    clgrjh %r15, %r1, .LBB9_1
+; CHECK-NEXT:  # %bb.2: # %entry
+; CHECK-NEXT:    .cfi_def_cfa_register %r15
+; CHECK-NEXT:    aghi %r1, 28672
+; CHECK-NEXT:    stg %r1, 0(%r15)
+; CHECK-NEXT:    mvhi 180(%r15), 0
+; CHECK-NEXT:    l %r0, 180(%r15)
+; CHECK-NEXT:    aghi %r15, 28672
+; CHECK-NEXT:    br %r14
+entry:
+  %stack = alloca [7122 x i32], align 4
+  %i = alloca i32, align 4
+  %0 = bitcast [7122 x i32]* %stack to i8*
+  %i.0.i.0..sroa_cast = bitcast i32* %i to i8*
+  store volatile i32 0, i32* %i, align 4
+  %i.0.i.0.6 = load volatile i32, i32* %i, align 4
+  ret void
+}
+
+
 declare i32 @foo()
 attributes #0 = {  "probe-stack"="inline-asm"  }
 
Index: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
===================================================================
--- llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
+++ llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
@@ -632,6 +632,7 @@
     static_cast<const SystemZInstrInfo *>(MF.getSubtarget().getInstrInfo());
   const SystemZSubtarget &STI = MF.getSubtarget<SystemZSubtarget>();
   const SystemZTargetLowering &TLI = *STI.getTargetLowering();
+  bool StoreBackchain = MF.getFunction().hasFnAttribute("backchain");
 
   MachineInstr *StackAllocMI = nullptr;
   for (MachineInstr &MI : PrologMBB)
@@ -677,8 +678,9 @@
     uint64_t LoopAlloc = ProbeSize * NumFullBlocks;
     SPOffsetFromCFA -= LoopAlloc;
 
-    BuildMI(*MBB, MBBI, DL, ZII->get(SystemZ::LGR), SystemZ::R1D)
-      .addReg(SystemZ::R15D);
+    if (!StoreBackchain)
+      BuildMI(*MBB, MBBI, DL, ZII->get(SystemZ::LGR), SystemZ::R1D)
+        .addReg(SystemZ::R15D);
     buildDefCFAReg(*MBB, MBBI, DL, SystemZ::R1D, ZII);
     emitIncrement(*MBB, MBBI, DL, SystemZ::R1D, -int64_t(LoopAlloc), ZII);
     buildCFAOffs(*MBB, MBBI, DL, -int64_t(SystemZMC::CallFrameSize + LoopAlloc),
@@ -700,6 +702,8 @@
     MBB = DoneMBB;
     MBBI = DoneMBB->begin();
     buildDefCFAReg(*MBB, MBBI, DL, SystemZ::R15D, ZII);
+    if (StoreBackchain)
+      emitIncrement(*MBB, MBBI, DL, SystemZ::R1D, int64_t(LoopAlloc), ZII);
 
     recomputeLiveIns(*DoneMBB);
     recomputeLiveIns(*LoopMBB);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D92803.310059.patch
Type: text/x-patch
Size: 2827 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201208/2c91d03b/attachment.bin>