[PATCH] D70097: [SCEV] Optimize SCEV cache usage

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Can you explain where the claimed speed-up is coming from?  We were missing cases in the caching before?  Is it simply not rebuilding the folding set node multiple times?  Something else?

This change appears to have a mix of functional change, and refactoring intermixed.  It would make review much easier if you were to split them apart.  In particular, a couple of suggestions:

1. The getOrCreateX functions appear to be easily implementable with the new infrastructure.  Please do so, as this avoids changes to the (delicate!) flag handling which will otherwise slow review.
2. Separate any removal/movement of caching routine calls into a separate change if possible.  (Not sure this is possible, which is the source of confusion.)



================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:1873
+    FoldingSetNodeID NodeID;
+    void *InsertPos = nullptr;
+
----------------
Having the insert point as part of the key seems odd structurally.  I'm also concerned about invalidation if the cache is otherwise modified between setting the IP and using it.  


================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:1880
+
+  template <class TExpr, class... TOps> struct SCEVCtor;
+
----------------
Looks like this forward declare can be in the cpp.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:1267
-  void *IP = nullptr;
-  if (const SCEV *S = UniqueSCEVs.FindNodeOrInsertPos(ID, IP)) return S;
-
----------------
It seems natural this should be before the rewrite rules?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70097