[PATCH] D93882: [SCEV] recognize logical and/or pattern

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune marked an inline comment as done.
aqjune added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7618
+      return EL0;
+    }
   }
----------------
nikic wrote:
> I think we can still do slightly better here: Even if the exact count is non-constant, we should still be able to take the min of the max (constant) counts in any case, right? In which case I would move this check into the `EitherMayExit` branch below and only adjust the BECount calculation, while keeping MaxBECount the same.
> 
> I think the condition you use here may also need more comment, as I would have expected the check to be only `!isa<SCEVConstant>(EL1.ExactNotTaken)` initially. Is the reasoning here that if EL0.ExactNotTaken is constant but EL1.ExactNotTaken is not, then either EL0.ExactNotTaken is zero, in which case umin will fold to zero and we won't use the second (potentially poison exit count), or it is non-zero, in which case we know that the other condition will be checked at least once and thus can't be poison.
> 
> So basically the general condition here would be `if (isKnownNotPoison(EL1.ExactNotTaken) || isKnownNonZero(EL0.ExactNotTaken) || isConstantZero(EL0.ExactNotTaken)`, where the last case is only safe if we do fold down to zero.
I think it depends on the meaning of `MaxNotTaken`. If it is always safe to introduce `llvm.assume(i < MaxNotTaken)` after exiting the loop, then the same rule will be applied, making `umin(EL0.MaxNotTaken, EL1.MaxNotTaken)` unsafe.
But, it *seems* `MaxNotTaken` is okay to be relaxed to allow poison because unlike `ExactNotTaken` it is unlikely to introduce such assumption or any equation that is equivalent to it. I imagined whether loop versioning would introduce something like that, but loop vectorization needs ExactNotTaken, and for other optimizations there might be other workarounds for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93882