[PATCH] D25810: [SCEV] Memoize visitMulExpr results in SCEVRewriteVisitor. Fix PR18606

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpressions.h:540
 
   /// Recursively visits a SCEV expression and re-writes it.
   template<typename SC>
----------------
lihuang wrote:
> sanjoy wrote:
> > Please document the fact that this only implements "pure" transforms.
> Sorry, but by "pure" transform do you mean identity transform? So what about:
> 
> /// Recursively visits a SCEV expression and re-writes an identical SCEV.
I meant a transform that maps equal inputs to equal outputs.  For instance, I cannot use this framework to implement a transform that maps all `SCEVConstant` s to `getConstant(Counter++)` since then the caching scheme won't work.


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpressions.h:560
+      auto *Result = SCEVVisitor<SC, const SCEV *>::visit(S);
+      RewriteResults.insert({S, Result});
+      return Result;
----------------
lihuang wrote:
> sanjoy wrote:
> > Just to be pedantic, I'd assert here that you did in fact insert a new entry.
> what about:
> 
> assert(RewriteResults.insert({S, Result}).second && "Should insert a new entry");
sgtm


https://reviews.llvm.org/D25810