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

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

Looks mostly good to me, but I would prefer we split this into two fixes, one for the realign bp algorithm change, another for the other.



================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1317
   };
-  // Used to probe realignment gap [stackptr - (stackptr % align), stackptr)
-  // when HasBP && isPPC64. In such scenario, normally we have r0, r1, r12, r30
-  // available and r1 is already copied to r30 which is BPReg. So BPReg stores
-  // the value of stackptr.
-  // First we have to probe tail interval whose size is less than probesize,
-  // i.e., [stackptr - (stackptr % align) % probesize, stackptr). At this stage,
-  // ScratchReg stores the value of ((stackptr % align) % probesize). Then we
-  // probe each block sized probesize until stackptr meets
-  // (stackptr - (stackptr % align)). At this stage, ScratchReg is materialized
-  // as negprobesize. At both stages, TempReg stores the value of
-  // (stackptr - (stackptr % align)).
+  // Used to probe stack that realignment is required.
+  // Following is pseudo code:
----------------
*that* -> *when*?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1318
+  // Used to probe stack that realignment is required.
+  // Following is pseudo code:
+  // final_sp = (sp & align) + negframesize;
----------------
Add the comments about atomic update required by ABI.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1322
+  // while (gap > probesize) {
+  //   stdux fp, probesize, sp
+  //   gap -= probesize;
----------------
This is inconsistent of what we generate below: using `stdu <backchain>, <negprobesize>(r1)`?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1325
+  // }
+  // sp -= gap;
+  // When HasBP && isPPC64, we have r0, r1, r12, r30 available and r1 is already
----------------
If we use `stdux above` in pseudo code, shouldn't we use `stdux here`?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1326
+  // sp -= gap;
+  // When HasBP && isPPC64, we have r0, r1, r12, r30 available and r1 is already
+  // copied to r30 which is BPReg. So BPReg stores the value of back-chain
----------------
Reorg this comment please. HasBP is always true, so this is really the difference for isPPC64 vs isPPC32?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1333
+  // `gap'.
   auto dynamicProbe = [&](MachineBasicBlock &MBB,
                           MachineBasicBlock::iterator MBBI, Register ScratchReg,
----------------
maybe rename `dynamicProbe`to  something like  `dynamicProbe_RealignWithBP` as this is only called with `HasBP && MaxAlign >1`?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1337
     assert(isPowerOf2_64(ProbeSize) && "Probe size should be power of 2");
-    // ScratchReg = stackptr % align
-    BuildMI(MBB, MBBI, DL, TII.get(PPC::RLDICL), ScratchReg)
-        .addReg(BPReg)
-        .addImm(0)
-        .addImm(64 - Log2(MaxAlign));
-    // TempReg = stackptr - (stackptr % align)
-    BuildMI(MBB, MBBI, DL, TII.get(PPC::SUBFC8), TempReg)
-        .addReg(ScratchReg)
-        .addReg(BPReg);
-    // ScratchReg = (stackptr % align) % probesize
-    BuildMI(MBB, MBBI, DL, TII.get(PPC::RLDICL), ScratchReg)
-        .addReg(ScratchReg)
-        .addImm(0)
-        .addImm(64 - Log2(ProbeSize));
+    assert(ProbeSize >= Subtarget.getRedZoneSize() &&
+           "Probe size should be larger or equal to the size of red-zone so "
----------------
This looks like a new limitation? Can we add a FIXME here?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1343
+    // FIXME: We only support NegProbeSize materialized by DForm currently.
+    // When HasBP && isPPC64, we should have a idle register to materialize
+    // NegProbeSize to larger values.
----------------
Do you mean we can use xform if we have an additional idle register?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1347
+    assert(isInt<16>(NegProbeSize) &&
+           "NegProbeSize should be materialized by DForm");
+    auto getBackChainPointer = [&](MachineBasicBlock &MBB,
----------------
*materialized* -> *materializable*?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1348
+           "NegProbeSize should be materialized by DForm");
+    auto getBackChainPointer = [&](MachineBasicBlock &MBB,
+                                   MachineBasicBlock::iterator MBBI,
----------------
Do we still need this lambda? Seems like only a ternary operator here?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1381
+      if (HasBackChainPointerInBPReg)
+        // Copy BPReg to FPReg to meet the definition of PROBED_STACKALLOC_64.
+        BuildMI(*ProbeExitMBB, ProbeExitMBB->end(), DL, CopyInst, TempReg)
----------------
`FPReg`? Typo? `TempReg`?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1408
+      allocateAndProbe(*ProbeLoopBodyMBB, ProbeLoopBodyMBB->end(), NegProbeSize,
+                       0, true, BackChainPointer);
+      BuildMI(ProbeLoopBodyMBB, DL, TII.get(isPPC64 ? PPC::ADDI8 : PPC::ADDI),
----------------
true /*UseDForm*/


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1509
+      // Probe residual part.
+      if (NegResidualSize) {
+        bool ResidualUseDForm = CanUseDForm(NegResidualSize);
----------------
Split change related to this into another patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100290