[PATCH] D40863: [AArch64][Darwin] Implement stack probing for static and dynamic stack objects

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Looks generally good to me, with nitpicks below. But I'd like to hear a comment on how this is supposed to be enabled (see comment below).



================
Comment at: lib/Target/AArch64/AArch64CallingConvention.td:348-349
 
+// Darwin stack probing function CSRs. Registers X9-X11 are used, LR since it's
+// a call.
+def CSR_AArch64_StackProbe_Darwin
----------------
>From the perspective of the caller I'd call it "clobbered" instead of "used".


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:462
+                                          MachineBasicBlock::iterator MBBI,
+                                          DebugLoc DL,
+                                          unsigned NumBytes) const {
----------------
I think DebugLoc will only ever be a default constructed one, which you can just do inside the function instead of passing it as a parameter.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:465-466
+  const AArch64Subtarget &Subtarget = MF.getSubtarget<AArch64Subtarget>();
+  const AArch64InstrInfo *TII =
+      static_cast<const AArch64InstrInfo *>(Subtarget.getInstrInfo());
+  const MachineFrameInfo &MFI = MF.getFrameInfo();
----------------
Isn't a `TargetInstrInfo` enough here so you can get away without a `static_cast`?


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:471-487
+  // If the LR has already been saved we don't need to save it before calling
+  // the probe function. However if it hasn't then the probe will clobber it.
+  bool LRIsSaved =
+      std::any_of(CSI.begin(), CSI.end(), [](const CalleeSavedInfo &SI) {
+        return SI.getReg() == AArch64::LR;
+      });
+  if (!LRIsSaved) {
----------------
Generally it feels sketchy to save/extend/restore the stackframe without MachineFrameInfo knowing about it. (For example you will hide this extra stuff from the `WarnStackSize` functionality in PrologueEpilogueINserter. That's probably not a big deal but if we can somehow find a better way that would be nice.

"We don't bother updating SP...", this is problematic, AFAIK a unix signal can come in at any time and will use your stack frame. It will probably work on platforms with stack red zones defined that the signal handlers have to respect.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:11040-11042
+  if (Subtarget->isTargetDarwin() && MF.getTarget().Options.EnableStackProbe)
+    return "___chkstk_darwin";
+  return "";
----------------
I assume this will be a correctness problem (on newer darwins?). So maybe we should not leave the decision to the frontend to set Options, but instead always make our decision based on the target triple (being darwin and newer than some version). Otherwise I can easily see non-clang frontends such as the various JITs we have around to miss the setting and fail.

You could then additionally provide a cl::opt then for cases where users want to explicitely disable the probe code generation for some reason.


================
Comment at: test/CodeGen/AArch64/arm64-stack-probing.ll:68-69
+
+attributes #0 = { noinline nounwind optnone uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="cyclone" "target-features"="+crypto,+fp-armv8,+neon,+zcm,+zcz" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="cyclone" "target-features"="+crypto,+fp-armv8,+neon,+zcm,+zcz" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
----------------
Did you try removing all the function attributes to make the test simpler?


Repository:
  rL LLVM

https://reviews.llvm.org/D40863