[PATCH] D113577: [SCEV] Support rewriting ZExt expressions with loop guard info.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

This generally looks very reasonable.

Can I request that you split off the change which switches the map from value keyed to SCEV keyed, and land that without separate review?  It seems like it should be entirely NFC.  Reducing diff and isolating failures is always good.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:13921
+  // sub-expressions.
+  if (RewriteMap.size() > 1) {
+    SmallVector<const SCEV *> ExprsToRewrite;
----------------
This should follow from the empty check just above.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:13934
+
   SCEVLoopGuardRewriter Rewriter(*this, RewriteMap);
   return Rewriter.visit(Expr);
----------------
You can fold this into the previous code by simply pushing Expr onto the replacement list last.  

Hm, do we need a fixed point in the loop above?  It seems like we have the possibility of one replacement enabling another.  Is there something non-obvious in the above which handles that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113577