[PATCH] D36087: [SCEV] Re-enable "Cache results of computeExitLimit"

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm

I've also verified that this does not break on the internal test case we had.



================
Comment at: include/llvm/Analysis/ScalarEvolution.h:1133
   /// Drop memoized information computed for S.
-  void forgetMemoizedResults(const SCEV *S);
+  void forgetMemoizedResults(const SCEV *S, bool EraseExitLimit = true);
 
----------------
Please add a comment on `EraseExitLimit`.  While the name is pretty self explanatory in terms of what it does, it isn't obvious in what situations it can safely be set to `false`.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:10858
+
+  if (EraseExitLimit)
+    for (auto I = ExitLimits.begin(), E = ExitLimits.end(); I != E;)
----------------
You don't have to fix this in this patch, I think we only need to do this if `S` has a `SCEVUnknown` somewhere in it.  Can you please add a TODO?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:10862
+        ExitLimits.erase(I++);
+      else
+        ++I;
----------------
Use braces to avoid the dangling else here.


https://reviews.llvm.org/D36087