[PATCH] D60420: [ValueTracking] Use computeConstantRange() for signed add overflow determination

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 6 inline comments as done.
nikic added a comment.

There are a few additional test changes due to the min/max handling in computeConstantRange() that has landed in the meantime.



================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4087-4091
+  ConstantRange CR1 = ConstantRange::fromKnownBits(Known, ForSigned);
+  ConstantRange CR2 = computeConstantRange(V, UseInstrInfo);
+  ConstantRange::PreferredRangeType RangeType =
+      ForSigned ? ConstantRange::Signed : ConstantRange::Unsigned;
+  return CR1.intersectWith(CR2, RangeType);
----------------
lebedev.ri wrote:
> So the only thing happening here is that `RangeType` is now being passed, right?
> Please, precommit the NFC part of the refactoring, and leave only
> ```
> + ConstantRange::PreferredRangeType RangeType =
> +     ForSigned ? ConstantRange::Signed : ConstantRange::Unsigned;
> - return CR1.intersectWith(CR2);
> + return CR1.intersectWith(CR2, RangeType);
> ```
Done in rL357968.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4136-4159
+  ConstantRange LHSRange = computeConstantRangeIncludingKnownBits(
+      LHS, /*ForSigned=*/true, DL, /*Depth=*/0, AC, CxtI, DT);
+  ConstantRange RHSRange = computeConstantRangeIncludingKnownBits(
+      RHS, /*ForSigned=*/true, DL, /*Depth=*/0, AC, CxtI, DT);
   OverflowResult OR =
       mapOverflowResult(LHSRange.signedAddMayOverflow(RHSRange));
   if (OR != OverflowResult::MayOverflow)
----------------
lebedev.ri wrote:
> This looks like it should be a new differential, after the this one.
This change is needed because the LHSKnown variable no longer exists, so it can't come after. There's no reason it can't come before though, so done in rL357969.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4195-4198
+  ConstantRange LHSRange = computeConstantRangeIncludingKnownBits(
+      LHS, /*ForSigned=*/true, DL, /*Depth=*/0, AC, CxtI, DT);
+  ConstantRange RHSRange = computeConstantRangeIncludingKnownBits(
+      RHS, /*ForSigned=*/true, DL, /*Depth=*/0, AC, CxtI, DT);
----------------
lebedev.ri wrote:
> And one more new differential
Done, I've reduced this to just the signed add case and will submit a followup for signed sub.


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

https://reviews.llvm.org/D60420