[PATCH] D17736: [SSP] Remove llvm.stackprotectorcheck.

[Eric Christopher via llvm-commits]

echristo added a comment.

Few inline comments and requests for more comments. Otherwise it's looking pretty nice.

Thanks!


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1939
@@ -1938,1 +1938,3 @@
+  const Value *IRGuard = TLI.getStackGuardAddr(M);
+  assert(IRGuard);
   SDValue GuardPtr = getValue(IRGuard);
----------------
Typical llvm style is: assert((cond) && "Some explanatory text")

(This happens in other places in your patch, I'll just include the first one)

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1361
@@ -1357,1 +1360,3 @@
     }
+    auto &SP = getAnalysis<StackProtector>();
+    if (dyn_cast<ReturnInst>(LLVMBB->getTerminator()) && SP.hasPrologue() &&
----------------
Comment.

================
Comment at: lib/CodeGen/StackProtector.cpp:202
@@ -202,1 +201,3 @@
   bool NeedsProtector = false;
+  for (const BasicBlock &BB : *F) {
+    for (const Instruction &I : BB) {
----------------
Standard style for this would be no braces for any of the conditionals or block.

================
Comment at: lib/CodeGen/TargetLoweringBase.cpp:1752
@@ +1751,3 @@
+
+bool TargetLoweringBase::supportsSelectionDAGSP() const {
+  return EnableSelectionDAGSP && !getTargetMachine().Options.EnableFastISel &&
----------------
Block comments on all of these if there aren't any in the .h file please? (I prefer more detailed comments on the implementation, but still).

================
Comment at: test/Assembler/auto_upgrade_intrinsics.ll:57
@@ -56,3 +56,2 @@
 ; CHECK: @llvm.objectsize.i32.p0i8
-; CHECK-DAG: declare i32 @llvm.objectsize.i32.p0i8
   %s = call i32 @llvm.objectsize.i32(i8* getelementptr inbounds ([60 x i8], [60 x i8]* @a, i32 0, i32 0), i1 false)
----------------
Hmm?


http://reviews.llvm.org/D17736