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

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpressions.h:552
+    // Currently only memoize results of visitMulExpr, because other
+    // calculations are much cheaper in the worst case.
+    DenseMap<const SCEV *, const SCEV *> MulRewriteResults;
----------------
Design-wise I'm not too happy to do see this happen only for multiplications -- have you considered over-riding the `visit` function and implementing the general logic there?


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpressions.h:585
     const SCEV *visitMulExpr(const SCEVMulExpr *Expr) {
+      auto it = MulRewriteResults.find(Expr);
+      if (it != MulRewriteResults.end())
----------------
s/`it`/`It`/


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpressions.h:592
+      auto *MulExpr = SE.getMulExpr(Operands);
+      MulRewriteResults[Expr] = MulExpr;
+      return MulExpr;
----------------
I'd use `.insert`.


https://reviews.llvm.org/D25810