[PATCH] D45439: [IRCE] Use NUW flag for indvar

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

In https://reviews.llvm.org/D45439#1065206, @samparker wrote:

> Hi Max,
>
> Thanks, the reason why I don't understand why nuw isn't used is because the range data !{i32 0, i32 2147483647} is used for %len.a and the loop is only entered if %len.a != 0. The loop then exits when %idx < 2, so surely we should be able to discern that nuw is applicable? isKnownNonNegativeInLoop is also not helping in this case, but it would seem there's enough information here. Could it be that SCEV not be using the range metadata in conjunction with the entry and exit conditions?
>
> thanks,
> sam


Wow, I've overlooked that it already has 1st iteration check. :) Well, we cannot always expect that SCEV is smart enough to do something, maybe in this particular case it simply failed to prove the non-negativity of the indvar. It's not a bug, SCEV is just now as smart as humans are. I will take a look into this case when I have some time, maybe we can improve the situation by teaching SCEV to derive `nuw` here. Feel free to take a look into it as well.

In https://reviews.llvm.org/D45439#1065630, @samparker wrote:

> Max,
>
> After starting and playing around for the day, I've got to the root question of: why do we do the wrap checks anyway? Aren't the following range checks in the IsIncreasing/Decreasing blocks enough?


Just omitting the `nsw` check is clearly wrong. Look at the logic for `ne` predicates.

  if (Pred == ICmpInst::ICMP_NE && LatchBrExitIdx == 1)
    // while (++i != len) {         while (++i < len) {
    //   ...                 --->     ...
    // }                            }
    // If both parts are known non-negative, it is profitable to use
    // unsigned comparison in increasing loop. This allows us to make the
    // comparison check against "RightSCEV + 1" more optimistic.
    if (SE.isKnownNonNegative(IndVarStart) &&
        SE.isKnownNonNegative(RightSCEV))
      Pred = ICmpInst::ICMP_ULT;
    else
      Pred = ICmpInst::ICMP_SLT;

This will not be an equivalent transform if  `i` is greater than `len` on 1st iteration but then overflows and can still reach `len` after that. So with `ne` we will make a lot of iterations and then exit the loop, and with `lt` we will fail on the 1st iteration.

However in other cases, when predicate was `slt/ult` from the beginning, it's an interesting question whether we need this condition or not. I will give it some thought and run fuzzer testing to see what happens, it should either expose some problems which are not obvious or give a clue that you might be right and we don't need NoWrap flag check at all.


https://reviews.llvm.org/D45439