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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:929
 
-    if (!HasNoSignedWrap(AR))
-      return false;
----------------
samparker wrote:
> samparker wrote:
> > samparker wrote:
> > > mkazantsev wrote:
> > > > mkazantsev wrote:
> > > > > Actually I look into it and the old implementation seems suspicious to me. We only check `nsw`, but when we don't have `nuw` and actually do have an unsigned wrap, why don't we have a bug on unsigned latch predicates? Or we do, but not aware about?.. :)
> > > > If you look at `HasNoSignedWrap`, it has a bit more complex check than just looking into the immediate flag, it also tries to go through `sext` to find this flag. Can we do the same for `nuw` and `zext` to keep things consistent?
> > > Is this case handled on line 915?
> > Ok, I'll make some changes. I also need to handle equality differently, because I believe its classed as unsigned which messes up the NoSignedWrap check.
> I'm trying to understand test_02 of decrementing_loop. The test looks like it should work, but I can't figure out how the transform is making the decision. If I try to check nuw on unsigned and nsw on signed values, this test stops being transformed because it figures the indvar is NSW but ULT is being used.
> Is this case handled on line 915?
No, I mean something like
  for (i = -10; i <u 10; i++)

This does have `nsw` but does not have a `nuw` because traversal from `-1` to `0` is an unsigned wrap. However in this case we will fail on 1st iteration, so it sholdn't be a bug in practice, but it's still fishy.



================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:929
 
-    if (!HasNoSignedWrap(AR))
-      return false;
----------------
mkazantsev wrote:
> samparker wrote:
> > samparker wrote:
> > > samparker wrote:
> > > > mkazantsev wrote:
> > > > > mkazantsev wrote:
> > > > > > Actually I look into it and the old implementation seems suspicious to me. We only check `nsw`, but when we don't have `nuw` and actually do have an unsigned wrap, why don't we have a bug on unsigned latch predicates? Or we do, but not aware about?.. :)
> > > > > If you look at `HasNoSignedWrap`, it has a bit more complex check than just looking into the immediate flag, it also tries to go through `sext` to find this flag. Can we do the same for `nuw` and `zext` to keep things consistent?
> > > > Is this case handled on line 915?
> > > Ok, I'll make some changes. I also need to handle equality differently, because I believe its classed as unsigned which messes up the NoSignedWrap check.
> > I'm trying to understand test_02 of decrementing_loop. The test looks like it should work, but I can't figure out how the transform is making the decision. If I try to check nuw on unsigned and nsw on signed values, this test stops being transformed because it figures the indvar is NSW but ULT is being used.
> > Is this case handled on line 915?
> No, I mean something like
>   for (i = -10; i <u 10; i++)
> 
> This does have `nsw` but does not have a `nuw` because traversal from `-1` to `0` is an unsigned wrap. However in this case we will fail on 1st iteration, so it sholdn't be a bug in practice, but it's still fishy.
> 
See my comment, I hope it will help. I think that either inserting the check `len.a != 0` before the loop or not expecting that IRCE will handle this case will be OK. I would prefer the first option of it works.


https://reviews.llvm.org/D45439