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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1465
+  Value *NewCond = Builder.CreateAnd(Conditions);
+  replaceExitCond(BI, NewCond, DeadInsts);
   return true;
----------------
nikic wrote:
> 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 :)
> That probably means that we need some one-use checks, otherwise we might unprofitable replicate a large condition tree.

Yes, this is also a legit option.

> I don't think it's the job of this transform to optimize ands with redundant operands...

But it just happens if we track duplicates, with no specific purpose... I mean, in (` (A & B)& (B & A`) we will only consider `A` and `B` once and therefore remove duplicates. Though, if we check one-use, this effect also disappers.

> There could be a mix of bitwise and logical and.

Let's leave logical operations for follow-up. I agree we should support them at some point, but they seem rare.


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

https://reviews.llvm.org/D139832