[PATCH] D81358: [PowerPC] Implement probing for dynamic stack allocation

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7879
+  if (hasInlineStackProbe(MF))
+    // Build a PROBED_ALLOCA node.
+    return DAG.getNode(PPCISD::PROBED_ALLOCA, dl, VTs, Ops);
----------------
Meaningless comment.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7881
+    return DAG.getNode(PPCISD::PROBED_ALLOCA, dl, VTs, Ops);
+  // Build a DYNALLOC node.
   return DAG.getNode(PPCISD::DYNALLOC, dl, VTs, Ops);
----------------
Meaningless comment.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11701
+  // attribute.
+  unsigned StackProbeSize = 4096;
+  const Function &Fn = MF.getFunction();
----------------
Can we set the size to `sys::Process::getPageSizeEstimate()`? Typical systems are all giving me 65536, are we probing too much if we set it to 4096 here?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11709
+  // so we constraint probe size not larger than 32768 now.
+  assert(StackProbeSize <= (1U << 15) && "Exceed ppc's current max probe size");
+  return StackProbeSize;
----------------
What happen with "stack-probe-size"="65536"?

The imm is known value, if we can NOT load an imm > 32768  with one instruction, we should be able to load it in two instructions. Why we have to assert here?




================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11710
+  assert(StackProbeSize <= (1U << 15) && "Exceed ppc's current max probe size");
+  return StackProbeSize;
+}
----------------
What about "stack-probe-size"="0"? When user would like to opt-out probe for some specific function? See https://reviews.llvm.org/rL225360`. We will now generate code divided by 0...

Also what about "stack-probe-size"="1"?  I think we should also round the size to stack alignment similar to Z.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11714
+// Lower dynamic stack allocation with probing. It's not implemented like
+// PPCRegisterInfo::lowerDynamicAlloc, since it involves loop and RegScavenger
+// is unable to allocate registers for loops in prologepilog.
----------------
I don't quite understand this. Why we can NOT do this similar to x86 or SystemZ? using PHI nodes.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11730
+  const unsigned ProbeSize = getStackProbeSize(*MF);
+  const BasicBlock *LLVM_BB = MBB->getBasicBlock();
+  MachineRegisterInfo &MRI = MF->getRegInfo();
----------------
`LLVM_BB` ? Why `LLVM`? Can we use more meaningful name?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11748
+  //       +---------+
+  // In MBB, calcute previous frame pointer and final stack pointer.
+  // In TestMBB, test if sp is equal to final stack pointer, if so, jump to
----------------
calculate?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81358