[PATCH] D104932: [instcombine] Fold overflow check using overflow intrinsic to comparison

[Sanjay Patel via Phabricator via llvm-commits]

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

LGTM - I'm not familiar with the LVI version, so it would be good to explain it here and/or point back to the original.

I committed one more icmp fold, so that should help reduce the risk of regressions:
0c400e895306 <https://reviews.llvm.org/rG0c400e8953069888315f85d62780839dccbaa33c>
I think we still want to find the signed predicate versions of those folds. I didn't check if the code here always manages to avoid those or if something else in instcombine already manages to squash them.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3098
+        // Compute the no-wrap range [X,Y) for LHS given RHS=C, then
+        // check for the inverted range using range offset trick.
+        ConstantRange NWR =
----------------
Describe the trick in the code comment?


================
Comment at: llvm/test/Transforms/InstCombine/smulo.ll:38
 ; CHECK-LABEL: @test_constant2(
-; CHECK-NEXT:    [[RES:%.*]] = tail call { i8, i1 } @llvm.smul.with.overflow.i8(i8 [[A:%.*]], i8 2)
-; CHECK-NEXT:    [[OVERFLOW:%.*]] = extractvalue { i8, i1 } [[RES]], 1
+; CHECK-NEXT:    [[TMP1:%.*]] = add i8 [[A:%.*]], 64
+; CHECK-NEXT:    [[OVERFLOW:%.*]] = icmp slt i8 [[TMP1]], 0
----------------
reames wrote:
> There might be another lurking fold here, but I'm not quite seeing it.
> 
> This is basically implementing a lookup table for the top two bits with the original mapping being:
> 00 = false
> 01 = true
> 10 = true
> 11 = false
> 
> We could use a shift and popct here, but that's slower.  Any tricks you can think of for this formulation?
> 
> One the later cases is also a region lookup test, but with 8 regions not 4.
> 
I don't see it either. I'm visualizing it as an abs() pattern, but that doesn't quite work since SMIN is not an overflow.


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

https://reviews.llvm.org/D104932