[PATCH] D139832: [IndVars] Support AND/OR in optimizeLoopExitWithUnknownExitCount

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1374
 
 std::optional<Value *> createReplacement(Value *V, const Loop *L,
                                          BasicBlock *ExitingBB,
----------------
nikic wrote:
> nit: Missing static.
https://github.com/llvm/llvm-project/commit/ba7af0bf693205fb82ff17d909c58e9402983fde


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1453
+      if (!match(V, m_Or(m_Value(LHS), m_Value(RHS))) &&
+          !match(V, m_LogicalOr(m_Value(LHS), m_Value(RHS))))
+        return false;
----------------
nikic wrote:
> LogicalOr also matches Or, no need to check both (same for LogicalAnd).
Didn't know that, done.


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1468
+  SmallPtrSet<Instruction *, 4> MayNeedUpdate;
+  MayNeedUpdate.insert(BI);
+  do {
----------------
nikic wrote:
> To check that I understand the purpose of the MayNeedUpdate set: We can't simply RAUW because we only check one-use on the and/or ops, but not on the icmps, so the icmp may still have other users that shouldn't be replaced?
> 
> If so, would it be possible to add the one-use check on the icmp as well, which would allow us to use simple RAUW, and also avoid duplicating the icmp?
Yes, you are right about the intention here. We can do it I guess, I don't really have a motivating counter-example where it would really matter.


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1481
+    if (auto Replaced = createReplacement(OldCond, L, ExitingBB, MaxIter,
+                                          SkipLastIter, SE, Rewriter)) {
+      Changed = true;
----------------
nikic wrote:
> nit: I would prefer passing in `Inverted` as a flag here rather than re-computing it in `createReplacement`. it took me a moment to understand that this code is not missing the inner condition inversion.
Do you mind if it goes as a follow-up? It will require some non-trivial restructuring there.


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

https://reviews.llvm.org/D139832