[PATCH] D59563: [ValueTracking] Compute range for abs without nsw

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Actually, i haven't fully noticed that there was that second test.
Looks good then modulo comment nit.



================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:5677-5678
+  if (R.Flavor == SelectPatternFlavor::SPF_ABS) {
+    // The result of abs(X) is >= 0, or SignedMin if the negation part of the
+    // abs (in RHS) has no NSW flag.
     Lower = APInt::getNullValue(BitWidth);
----------------
This comment reads weird.
I think you want something more like:
```
// If the negation part of the abs (in RHS) has NSW flag,
// then the result of abs(X) is [0..SIGNED_MAX],
// else it is [0..-SIGNED_MIN],
```
?


================
Comment at: llvm/test/Transforms/InstSimplify/icmp-abs-nabs.ll:158
   %abs = select i1 %cmp, i32 %negx, i32 %x
   %r = icmp ult i32 %abs, 2147483649
   ret i1 %r
----------------
nikic wrote:
> lebedev.ri wrote:
> > Hm, after thinking about it a bit more, i think the test coverage could be improved.
> > In reality, this test only shows that we end up folding this pattern to `true`.
> > But what are the thresholds for that opt to happen?
> > I.e. i think you'd want to add two more test cases here, with `2147483649+1` and `2147483649-1` constants.
> The test directly below this one checks the <2147483648 case, which does not fold. So if <2147483648 does not fold but <2147483649 does, this has to be the upper boundary of the range. (It does not check the lower boundary, but as it's zero and >= 0 in unsigned domain is tautological we can't do a direct check for it.)
Okay, https://rise4fun.com/Alive/tya


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59563