[PATCH] D158084: [AArch64] Stack probing for function prologues

[Adhemerval Zanella via Phabricator via llvm-commits]

zatrazz added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1855
+    bool NeedsStackProbe = TLI.hasInlineStackProbe(MF) &&
+                           (NumBytes > TLI.getStackProbeMaxUnprobedStack(MF) ||
+                            MFI.hasVarSizedObjects());
----------------
I am not sure if using getStackProbeMaxUnprobedStack as the threshold for emiting static probes is the more optimized value. GCC uses getStackProbeSize - 1024, since with defined invariants (the stack has been probed at [sp, #N] with 0 <= N <= 1024) rest of the guard page can be used without further probing. Afaiu the 1024 was chosen as value to minimize as possible the required probes based on common function stack requirements.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:4189
+
+static const unsigned STACK_PROBE_LOOP_UNROLL = 3;
+
----------------
Should we follow GCC as well here (it defines the the unroll value as 5)? And does it worth to add a tunable for this?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.h:955
+  unsigned getStackProbeMaxUnprobedStack(const MachineFunction &MF) const {
+    return 1024;
+  }
----------------
Does it need to be a function? This value should not vary based on function, so maybe it would be better to move a constant.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:8532
+      .addReg(ScratchReg)
+      .addImm(0)
+      .setMIFlags(MachineInstr::FrameSetup);
----------------
I don't think this does what the comments above specifies, since on static_4096 testcase the produced code does 'str     xzr, [sp]' where it should be 'str     xzr, [sp, #1024]'. I think ou should 'TLI->getStackProbeMaxUnprobedStack(MF) /8'.




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

https://reviews.llvm.org/D158084