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

[Evgeniy Stepanov via llvm-commits]

eugenis added inline 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);
----------------
pcc wrote:
> Why did you change this? The comment was correct before, I believe?
I don't recall changing this. Weird.
Reverted.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:187
@@ +186,3 @@
+  ConstantRange AccessStartRange =
+      SE->getUnsignedRange(Expr).intersectWith(SE->getUnsignedRange(Expr));
+  ConstantRange SizeRange =
----------------
pcc wrote:
> Isn't this equivalent to just `SE->getUnsignedRange(Expr)`?
The second one is supposed to be getSignedRange. This is actually quite inefficient as these calls do a lot of duplicate work.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:302
@@ +301,3 @@
+
+      default:
+        if (Visited.insert(I).second)
----------------
pcc wrote:
> Should this be the default behaviour? It seems like it would be safer to use explicit whitelisting of instructions as the old code does.
This sounds like a awful lot of whitelisting. Unlike the previous algorithm where we explicitly supported a few instructions, ScalarEvolution can handle almost anything, and if not it would just say that the access range is full-set.

I think blacklisting would be a better approach.

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


Repository:
  rL LLVM

http://reviews.llvm.org/D14599