[PATCH] D14599: [safestack] Rewrite isAllocaSafe using SCEV.

[Peter Collingbourne via llvm-commits]

pcc added a comment.

This is great, thanks for working on this. A few mostly minor comments.


================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:111
@@ -183,1 +110,3 @@
+  /// \brief Build a constant representing a pointer to the unsafe stack
+  /// pointer.
   Value *getOrCreateUnsafeStackPtr(IRBuilder<> &IRB, Function &F);
----------------
Why did you change this? The comment was correct before, I believe?

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:187
@@ +186,3 @@
+  ConstantRange AccessStartRange =
+      SE->getUnsignedRange(Expr).intersectWith(SE->getUnsignedRange(Expr));
+  ConstantRange SizeRange =
----------------
Isn't this equivalent to just `SE->getUnsignedRange(Expr)`?

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:189
@@ +188,3 @@
+  ConstantRange SizeRange =
+      ConstantRange(APInt(BitWidth, 0, false), APInt(BitWidth, Size, false));
+  ConstantRange AccessRange = AccessStartRange.add(SizeRange);
----------------
Nit: you don't need to supply a third argument here (and below); `false` is the default.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:302
@@ +301,3 @@
+
+      default:
+        if (Visited.insert(I).second)
----------------
Should this be the default behaviour? It seems like it would be safer to use explicit whitelisting of instructions as the old code does.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:597-598
@@ -531,1 +596,4 @@
+  SE = &getAnalysis<ScalarEvolutionWrapperPass>().getSE();
+  // F.dump();
+  // SE->print(errs());
 
----------------
Did you leave this in intentionally?


Repository:
  rL LLVM

http://reviews.llvm.org/D14599