[PATCH] D100290: [PowerPC] Make sure the first probe is full size or is the last probe when stack is realigned

[Jinsong Ji via Phabricator via llvm-commits]

jsji accepted this revision as: jsji.
jsji added a comment.
This revision is now accepted and ready to land.

LGTM , with some comments on comments.



================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1348
+           "NegProbeSize should be materialized by DForm");
+    auto getBackChainPointer = [&](MachineBasicBlock &MBB,
+                                   MachineBasicBlock::iterator MBBI,
----------------
jsji wrote:
> Do we still need this lambda? Seems like only a ternary operator here?
`we should have a idle register to materialize` -> ` we can use xform if we have an additional idle register`


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1330
+  //
+  // When HasBP && HasRedzone, we have r0, r1, r12, r30 available and r1 is
+  // already copied to r30 which is BPReg. So BPReg stores the value of
----------------
How about?
```
When HasBP & HasRedzone, back-chain pointer is already saved in BPReg before probe code, we don't need to save it, so we get one additional reg that can be used to materialize the probeside if needed to use xform.
Otherwise, we can NOT materialize probeside, so we can only use Dform for now. 

The allocations are:
if(HasBP & HasRedzone){
r0: materialize the probesize if needed so that we can use xform.
r12: `neg_gap`
}
else{
r0: back-chain pointer
r12: `neg_gap`.
}

```


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1337
+  // `neg_gap'.
+  auto probeStackWhenRealigned = [&](MachineBasicBlock &MBB,
+                                     MachineBasicBlock::iterator MBBI,
----------------
`probeStackWhenRealigned` -> `probeRealignedStack`


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1352
+    // FIXME: We only support NegProbeSize materializable by DForm currently.
+    // When HasBP && HasRedzone, we should have a idle register to materialize
+    // NegProbeSize to larger values.
----------------
`probeStackWhenRealigned` is called only under `if (HasBP && MaxAlign > 1) {` , 
so the `HasBP` should be expected to be always true in this function,
I think we can safely remove it from the condition? (you can add an assert if you want).


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1382
+      if (HasBP && HasRedZone)
+        // Copy BPReg to TempReg to meet the definition of PROBED_STACKALLOC_64.
+        BuildMI(*ProbeExitMBB, ProbeExitMBB->end(), DL, CopyInst, TempReg)
----------------
`PROBED_STACKALLOC_64` assumes Operand(1) stores the old sp, copy BPReg to TempReg to satisfy it.


================
Comment at: llvm/test/CodeGen/PowerPC/stack-clash-prologue-nounwind.ll:337
+; CHECK-LE-NEXT:    lis r12, 4
+; CHECK-LE-NEXT:    ori r12, r12, 0
+; CHECK-LE-NEXT:    mtctr r12
----------------
Looks like this can be ignored if we found the imm is `0`. Can be fixed in a follow up patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100290/new/

https://reviews.llvm.org/D100290