[PATCH] D34387: [PATCH 2/2] Implement "probe-stack" on x86

[David Majnemer via Phabricator via llvm-commits]

majnemer added inline comments.


================
Comment at: lib/CodeGen/MachineFunction.cpp:216-224
+/// Whether we should be probing the stack for the function.
+///
+/// Probing the stack means that we must read or write to the stack on every
+/// page. This is to ensure that a guard page will be hit and stack overflow
+/// can be detected. We insert instructions to do this when allocating from
+/// the stack.
+bool MachineFunction::shouldProbeStack() const {
----------------
I think this should be renamed or change its definition to reflect its name, we will probe the stack on x86 Windows regardless of the whether or not this attribute was supplied.

Perhaps `hasProbeStackAttr`?


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:801
 
-  const char *Symbol;
-  if (Is64Bit) {
+  std::string Symbol;
+  if (MF.getFunction()->hasFnAttribute("probe-stack")) {
----------------
Could this be a `StringRef`?


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1034-1035
+  bool UseRedZone = false;
+  bool UseStackProbe =
+      (STI.isOSWindows() && !STI.isTargetMachO()) || MF.shouldProbeStack();
 
----------------
You could make a function which gets the stack probe symbol name, then UseStackProbe could be defined in terms of that name being non-empty.


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1255-1257
+    // In the large code model, we have to load the stack probe function into a scratch
+    // register to call it. R11 is used for that.
+    bool SpillR11 = Is64Bit && MF.getTarget().getCodeModel() == CodeModel::Large;
----------------
Test case for this?


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1263-1264
+
+    pushRegForStackProbeCall(MF, MBB, MBBI, DL, RAXAlive, X86::RAX, NumBytes);
+    pushRegForStackProbeCall(MF, MBB, MBBI, DL, RBXAlive, X86::RBX, NumBytes);
+    if (SpillR11) {
----------------
Saving RBX seems new. I'd have a comment describing both of these saves.


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1271
+      BuildMI(MBB, MBBI, DL, TII.get(X86::MOV32ri), X86::EDX)
+           .addImm(0x1000)
+           .setMIFlag(MachineInstr::FrameSetup);
----------------
Magic number should have a comment or stuck in a const variable with a nice name. Even better would be both.


https://reviews.llvm.org/D34387