[PATCH] D59668: [ValueTracking] Take signedness into account for and/or ranges

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/test/Transforms/InstSimplify/icmp-constant.ll:495-503
 ; -75 = 0b10110101, 53 = 0b00110101
 define i1 @and3_true1(i8 %x) {
 ; CHECK-LABEL: @and3_true1(
-; CHECK-NEXT:    [[Y:%.*]] = and i8 [[X:%.*]], -75
-; CHECK-NEXT:    [[Z:%.*]] = icmp sge i8 [[Y]], -75
-; CHECK-NEXT:    ret i1 [[Z]]
+; CHECK-NEXT:    ret i1 true
 ;
   %y = and i8 %x, -75
   %z = icmp sge i8 %y, -75
----------------
nikic wrote:
> lebedev.ri wrote:
> > This is a miscompile, i believe.
> > https://rise4fun.com/Alive/R6k
> > ```
> > ----------------------------------------
> > Optimization: 1
> > 
> > ERROR: Mismatch in values of i1 %z
> > 
> > Example:
> > %x i8 = 0x80 (128, -128)
> > %y i8 = 0x80 (128, -128)
> > Source value: 0x0 (0)
> > Target value: 0x1 (1, -1)
> > ```
> Wow, thanks for catching that! What I did with the lower bound for `and` here doesn't make sense, it should just be SignedMin, independent of the constant.
> 
> I'm wondering if I should maybe move these calculations to ConstantRange, so they can be independently (and exhaustively) tested. The calculations here are quite specific though, in that they compute the range when combining a full range and a single constant. Something like `ConstantRange::forBinaryAndWithConst()` and friends?
I'm not quite sure how to best expose that (`ConstantRange::forBinaryAndWithConst()`
is a reasonable intermediate solution), but i *really* *really* like the idea of
separating them into small functions with exhaustive test coverage.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59668