[PATCH] D129630: [SCEVExpander] Make CanonicalMode handing in isSafeToExpand() more robust (PR50506)

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h:41
+/// CanonicalMode indicates whether the expander will be used in canonical mode.
 bool isSafeToExpandAt(const SCEV *S, const Instruction *InsertionPoint,
+                      ScalarEvolution &SE, bool CanonicalMode);
----------------
It looks like only 2 uses of those versions remain. I think it would be good to replace them with the member variants, to reduce the chance of mis-use. Although the use in LSR might require a bit of extra work :(


================
Comment at: llvm/lib/Transforms/Scalar/LoopPredication.cpp:526
     if (!SE->isLoopInvariant(Op, L) ||
-        !isSafeToExpandAt(Op, Preheader->getTerminator(), *SE))
+        !isSafeToExpandAt(Op, Preheader->getTerminator(), *SE,
+                          /* CanonicalMode */ true))
----------------
Could we just pass the used Expander and then use `Expander.isSafeToExpandAt` here?


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:3339
+        if (SE.isLoopInvariant(N, L) &&
+            isSafeToExpand(N, SE, /* CanonicalMode */ false) &&
             (!NV->getType()->isPointerTy() ||
----------------
It might be worth creating the `SCEVExpander` up-front and then use the member version?


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

https://reviews.llvm.org/D129630