[PATCH] D111533: [SCEV] Invalidate user SCEVs along with operand SCEVs to avoid cache corruption

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

I think this makes sense, and is worth the remaining compile time cost.  If you want to rebase over the other patch (see my suggestion there), and address the minor style comments, I'd be willing to LGTM.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:1104
     addToLoopUseLists(S);
+    registerUser(S, { Op });
     return S;
----------------
Please address the lint check.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:4103-4106
+    if (!SCEVUsers.count(Op)) {
+      SmallPtrSet<const SCEV *, 16> Users;
+      SCEVUsers[Op] = std::move(Users);
+    }
----------------
danilaml wrote:
> Why is this `if` needed?
Yeah, the default constructor should handle this case if I'm reading it right.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:12810
+void ScalarEvolution::forgetMemoizedResults(const SCEV *S) {
+  SmallPtrSet<const SCEV *, 16> Visited;
+  SmallVector<const SCEV *, 16> Worklist;
----------------
I'm not sure this is the right place for the use list walk.  Doing it here has the effecting of leaving all the user SCEVs around, and simplify forgetting the cached property of said SCEVs.  Is that actually what we want?  In particular, the (transitively reached) SCEVs will still be in the ValueExprMap and thus still mapped to the instructions.  

Maybe we should be forgetting all the transitive inst->SCEV mappings as well?  If we've, e.g., added flags to the IR instruction, that could very well map to a new SCEV node. 

Hm, thinking about it, that's a deeper change than it first sounded.  So yes, I'm convinced this is a reasonable starting point.  :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111533/new/

https://reviews.llvm.org/D111533