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

[Li Huang via llvm-commits]

lihuang added inline comments.


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpressions.h:540
 
   /// Recursively visits a SCEV expression and re-writes it.
   template<typename SC>
----------------
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.


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpressions.h:560
+      auto *Result = SCEVVisitor<SC, const SCEV *>::visit(S);
+      RewriteResults.insert({S, Result});
+      return Result;
----------------
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");


================
Comment at: test/Analysis/ScalarEvolution/pr18606.ll:3
+
+; CHECK: main
+
----------------
sanjoy wrote:
> Please add a basic output sanity check.
Okay.


https://reviews.llvm.org/D25810