[PATCH] D81460: [PowerPC] Implement probing for prologue

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:678
+// allocation in prologue MBB.
+bool PPCFrameLowering::findCRRegister(MachineBasicBlock *MBB,
+                                      Register *CRReg) const {
----------------
We are in prologue,  there shouldn't be any use of cr before this, why can't we simply always use `CR0` here?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1053
 
-  // This condition must be kept in sync with canUseAsPrologue.
-  if (HasBP && MaxAlign > 1) {
-    if (isPPC64)
-      BuildMI(MBB, MBBI, dl, TII.get(PPC::RLDICL), ScratchReg)
-          .addReg(SPReg)
-          .addImm(0)
-          .addImm(64 - Log2(MaxAlign));
-    else // PPC32...
-      BuildMI(MBB, MBBI, dl, TII.get(PPC::RLWINM), ScratchReg)
-          .addReg(SPReg)
-          .addImm(0)
-          .addImm(32 - Log2(MaxAlign))
-          .addImm(31);
-    if (!isLargeFrame) {
-      BuildMI(MBB, MBBI, dl, SubtractImmCarryingInst, ScratchReg)
-        .addReg(ScratchReg, RegState::Kill)
+  if (TLI.hasInlineStackProbe(MF) && FrameSize > TLI.getStackProbeSize(MF)) {
+    BuildMI(MBB, MBBI, dl,
----------------
Can we add comments about free probe?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1055
+    BuildMI(MBB, MBBI, dl,
+            TII.get(isPPC64 ? PPC::PROBED_STACKALLOC_64
+                            : PPC::PROBED_STACKALLOC_32))
----------------
Add comment about why we need this pseudo? and when it will be handled.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1383
+  MachineFrameInfo &MFI = MF.getFrameInfo();
+  auto Where = llvm::find_if(PrologMBB, [](MachineInstr &MI) {
+    int Opc = MI.getOpcode();
----------------
`Where` -> `StackAllocAIPos` ?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1418
+  // can split into two parts,
+  // 1. SP = SP - SP % MaxAlign
+  // 2. SP = SP + NegFrameSize
----------------
Comments/code mismatch? We are generating `RLDICL`/`RLWINM` below, please add comments about the calculation as well..


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1448
+    bool FoundCRReg = findCRRegister(&PrologMBB, &CRReg);
+    assert(FoundCRReg && "Can't find available cr register");
+    (void)FoundCRReg;
----------------
What if we can't find CR and we don't build with assert on?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1457
+    else {
+      BuildMI(PrologMBB, {MI}, DL, TII.get(isPPC64 ? PPC::LIS8 : PPC::LIS),
+              ScratchReg)
----------------
There are several instance of similar code, maybe worth a NFC refactor first.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1470
+    // Probe residual part.
+    if (NegResidualSize)
+      allocateAndProbe(PrologMBB, {MI}, NegResidualSize);
----------------
We have to check. and do Residual block regardless of NumBlocks, please move it out of `if/else`.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1479
+    // Synthesize loop body.
+    LoopMBB->addLiveIn(SPReg);
+    LoopMBB->addLiveIn(ScratchReg);
----------------
Shall we `recomputeLiveIns` for both `LoopMBB` and `ExitBB`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81460