[PATCH] D88078: [PowerPC] Probe the gap between stackptr and realigned stackptr

[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.  Some comment update please.



================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1382
+  uint64_t ProbeSize = TLI.getStackProbeSize(MF);
+  int64_t NegProbeSize = -(int64_t)ProbeSize;
   assert(isInt<32>(NegProbeSize) && "Unhandled probe size");
----------------
`getStackProbeSize` returns `unsigned`. 
So this should still be safe , as we won't have large uint64 ProbeSize that may become negative when casting to `int64_t`.
However, this looks like a potential trap to me.

Maybe we should use `unsigned` for `ProbeSize`?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1454
+  auto dynamicProbe = [&](MachineBasicBlock &MBB,
+                          MachineBasicBlock::iterator MBBI, Register ScratchReg,
+                          Register TempReg) {
----------------
Document `ScratchReg` and `TempReg` please. And also document the assumption, eg: stackptr is in `BPReg` etc.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1458
+    assert(isPowerOf2_64(ProbeSize) && "Probe size should be power of 2");
+    // stackptr % align
+    BuildMI(MBB, MBBI, DL, TII.get(PPC::RLDICL), ScratchReg)
----------------
// ScratchReg = stackptr % align


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1463
+        .addImm(64 - Log2(MaxAlign));
+    // stackptr - (stackptr % align)
+    BuildMI(MBB, MBBI, DL, TII.get(PPC::SUBFC8), TempReg)
----------------
// TempReg = stackptr - (stackptr % align)


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1467
+        .addReg(BPReg);
+    // (stackptr % align) % probesize
+    BuildMI(MBB, MBBI, DL, TII.get(PPC::RLDICL), ScratchReg)
----------------
// ScratchReg = (stackptr % align) % probesize


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88078