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

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 4 inline comments as done.
nikic added inline comments.


================
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:
> nikic wrote:
> > 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.
> Sure. But i was talking about the entire block of code i highlighted with a comment.
> I.e. i think the rest of the `computeOverflowForSignedAdd()` change should be in a new diff too.
> Or if you leave just the `computeConstantRangeIncludingKnownBits()` part, there are no test changes?
Ooooh, I didn't realize that you can highlight a range of code and assumed your comment is about the change directly above.

But yes, just the computeConstantRangeIncludingKnownBits() change doesn't result in test changes (it it not strictly speaking NFC, but it's likely not possible to actually trigger a difference without using the function in a signed context).


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

https://reviews.llvm.org/D60420