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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1452
+
+  if (ExitIfTrue && Conditions.size() > 1)
+    // For AND aggregations, only consider true branch staying in loop.
----------------
I think the structure here isn't quite right. The `L->contains(FalseSucc)` check from createReplacement() should be in this function and just pass in an inversion flag. Then here, if we invert, we need to handle ORs, otherwise we need to handle ANDs.

Again, I think parity between and/or needs to be either in the initial patch, or in the patch stack.


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1465
+  Value *NewCond = Builder.CreateAnd(Conditions);
+  replaceExitCond(BI, NewCond, DeadInsts);
   return true;
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > nikic wrote:
> > > Why do we have to reconstruct the whole and chain? Can't we replace a single condition only? This looks quite fragile, especially as we probably want to generalize this to logical and as well, and at that point we'd have to replicate different types of ands in the correct position.
> > It's only possible if no one else outside our cluster is using it. If they are, we can't simply replace operands of AND's.
> > 
> > One more (indirect, but still useful) effect of this algorithm is that it'll also eliminate duplicates, should they be in the AND tree.
> > 
> > As for logical AND -- what's the complexity? We keep conditions in the original order in `Conditions`, and replace them in the very same places. All it takes to support logical AND -- is to call another Builder function (multi-operand logical AND). If it doesn't exist, it's only a reason to write it.
> "using them" I mean "using ANDs"
> It's only possible if no one else outside our cluster is using it. If they are, we can't simply replace operands of AND's.

That probably means that we need some one-use checks, otherwise we might unprofitable replicate a large condition tree.

> One more (indirect, but still useful) effect of this algorithm is that it'll also eliminate duplicates, should they be in the AND tree.

I don't think it's the job of this transform to optimize ands with redundant operands...
 
> As for logical AND -- what's the complexity? We keep conditions in the original order in `Conditions`, and replace them in the very same places. All it takes to support logical AND -- is to call another Builder function (multi-operand logical AND). If it doesn't exist, it's only a reason to write it.

There could be a mix of bitwise and logical and. Of course, it is safe to replace everything with logical and, it's just non-optimal.

In any case, I believe logical and handling should be part of the initial patch (or at least part of the patch stack). We should avoid differences in support, they will come back to bite us at some point :)


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

https://reviews.llvm.org/D139832