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

Nikita Popov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Apr 9 00:27:59 PDT 2019


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





More information about the llvm-commits mailing list