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

Jinsong Ji via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 30 14:09:14 PDT 2020


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

LGTM.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11701
+  // attribute.
+  unsigned StackProbeSize = 4096;
+  const Function &Fn = MF.getFunction();
----------------
lkail wrote:
> jsji wrote:
> > 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?
> Yes, `sysconf` gives `65536` on my rhel POWER machine and `4096` on my rhel X86 machine. I'm quite concerned if users do cross compilation if the host has large page size than the target.
How about we introduce an option for user to switch the default value here, eg: default is still the smallest, 4096, but we provide another choice to use system default (65536 most of the time) .


================
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.
----------------
lkail wrote:
> jsji wrote:
> > I don't quite understand this. Why we can NOT do this similar to x86 or SystemZ? using PHI nodes.
> The comment to compare with `PPCRegisterInfo::lowerDynamicAlloc` might be a little bit misleading. My point is `PPCTargetLowering::emitProbedAlloca` is unable to expaned in `prologepilog` like `PPCRegisterInfo::lowerDynamicAlloc`. Should I delete this comparison?
> 
> Looks we don't need phi nodes in the loop, only `r1`, which is a physical register, is updated.
yeah, maybe remove the comments comparing to `lowerDynamicAlloc`. 


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp:614
+    MachineBasicBlock::iterator II) const {
+  // Get the instruction.
+  MachineInstr &MI = *II;
----------------
Meaningless comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81358





More information about the llvm-commits mailing list