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

[David Majnemer via Phabricator via llvm-commits]

majnemer added inline comments.


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:510
+                                               DebugLoc DL,
+                                               bool &IsAlive,
+                                               unsigned RegType,
----------------
Why is this by ref?


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:801
 
-  const char *Symbol;
-  if (Is64Bit) {
-    if (STI.isTargetCygMing()) {
-      Symbol = "___chkstk_ms";
-    } else {
-      Symbol = "__chkstk";
-    }
-  } else if (STI.isTargetCygMing())
-    Symbol = "_alloca";
-  else
-    Symbol = "_chkstk";
+  StringRef Symbol(STI.getTargetLowering()->getStackProbeSymbolName(MF));
 
----------------
I think it is more natural to see: StringRef Symbol = STI.getTargetLowering()->getStackProbeSymbolName(MF);


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1243-1245
+    // 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;
----------------
Am I correct in thinking that we will save and restore R11 when we previously would not in the large code model for Win64? If so, was that a bug in the implementation of the large code model for Win64?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:36342
+/// string if not applicable.
+const StringRef X86TargetLowering::getStackProbeSymbolName(MachineFunction &MF) const {
+  // If the function specifically requests stack probes, emit them.
----------------
const StringRef looks weird, StringRefs are immutable.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:36344-36347
+  if (MF.getFunction()->hasFnAttribute("probe-stack")) {
+    return MF.getFunction()->getFnAttribute("probe-stack").getValueAsString();
+  }
+
----------------
I'd kill the braces here.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:36355-36357
+  if (Subtarget.is64Bit()) {
+    return Subtarget.isTargetCygMing() ? "___chkstk_ms" : "__chkstk";
+  }
----------------
Ditto.


================
Comment at: lib/Target/X86/X86ISelLowering.h:1059
 
+    const StringRef getStackProbeSymbolName(MachineFunction &MF) const override;
+
----------------
Ditto regarding const StringRef.


https://reviews.llvm.org/D34387