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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


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


================
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;
----------------
LogicalOr also matches Or, no need to check both (same for LogicalAnd).


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1468
+  SmallPtrSet<Instruction *, 4> MayNeedUpdate;
+  MayNeedUpdate.insert(BI);
+  do {
----------------
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?


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1481
+    if (auto Replaced = createReplacement(OldCond, L, ExitingBB, MaxIter,
+                                          SkipLastIter, SE, Rewriter)) {
+      Changed = true;
----------------
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.


================
Comment at: llvm/test/Transforms/IndVarSimplify/turn-to-invariant.ll:299
 
 ; TODO: Simplified version 2 of test_litter_conditions.
 define i32 @test_litter_conditions_02(i32 %start, i32 %len) {
----------------
nit: Drop TODO


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

https://reviews.llvm.org/D139832