[PATCH] D17736: [SSP, 1/2] Refactor to support customizable stack guard load from IR level.

[Kyle Butt via llvm-commits]

iteratee added a comment.

Some comments, mostly about comments and documentation.


================
Comment at: include/llvm/IR/Intrinsics.td:329
@@ +328,3 @@
+// target supports SSP in SelectionDAG. Otherwise SSP pass will generate IR
+// level code, including experimental_stackguardvalue calls. Backends may lower
+// experimental_stackguardvalue by overriding getStackGuardAddr and implementing
----------------
I agree with eric that this shouldn't be experimental.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5376
@@ +5375,3 @@
+    // SPDescriptor expects either an address, or an intrinsic call, so we need
+    // to strip the LoadInst if possible.
+    if (auto Load = dyn_cast<LoadInst>(I.getArgOperand(0))) {
----------------
I'm not sure I understand this comment.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:550
@@ -550,3 +549,3 @@
 
-    /// The guard variable which we will compare against the stored value in the
-    /// stack protector stack slot.
+    /// It either stores an address to the stack guard, or stores a
+    /// llvm.experimental_stackguardvalue CallInst.
----------------
Is this still accurate? Are we keeping support for loading an address?

================
Comment at: lib/CodeGen/StackProtector.cpp:431
@@ -417,3 +430,3 @@
       //     ...
-      //     %1 = load __stack_chk_guard
+      //     %1 = <value of the stack guard>
       //     %2 = load StackGuardSlot
----------------
I think this comment should read %1 = <load or intrinsic to read stack guard>

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:10104
@@ -10103,5 +10103,3 @@
 
-bool AArch64TargetLowering::useLoadStackGuardNode() const {
-  return true;
-}
+bool AArch64TargetLowering::forceLoadStackGuardNode() const { return true; }
 
----------------
Can you add a comment about why this needs to be overridden for this platform?


http://reviews.llvm.org/D17736