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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

This looks basically good to me, only concern is about the instruction move.



================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1446
+    // We do not want to duplicate these operations, but simply replace their
+    // operands. It is only legal when no one else is using them.
+    if (Inverted) {
----------------
This comment is out of place now (I think it can just be omitted, you have another below).


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1464
+    // Avoid instruction duplication.
+    if (isa<Instruction>(Curr) && !Curr->hasOneUse())
+      return false;
----------------
I don't think we need to check `isa<Instruction>(Curr) &&` here (I guess you could have a constant expression icmp or something, but the whole transform doesn't make sense for that case).


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1480
+        NCI->setName(OldCond->getName() + ".first_iter");
+        NCI->moveBefore(cast<Instruction>(OldCond));
+      }
----------------
Should we be setting the insert point on Rewriter instead? Mainly wondering if we might generate multiple insts and this then only moves one of them.


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

https://reviews.llvm.org/D139832