[PATCH] D59473: [ValueTracking] Avoid redundant known bits calculation in computeOverflowForSignedAdd()

[Nikita Popov via Phabricator via llvm-commits]

nikic marked an inline comment as done.
nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4192-4194
+    KnownBits AddKnown(LHSKnown.getBitWidth());
+    computeKnownBitsFromAssume(
+        Add, AddKnown, /*Depth=*/0, Query(DL, AC, CxtI, DT, true));
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > nikic wrote:
> > > > lebedev.ri wrote:
> > > > > What about this comment in `computeKnownBits()`:
> > > > > ```
> > > > >   // computeKnownBitsFromAssume strictly refines Known.
> > > > >   // Therefore, we run them after computeKnownBitsFromOperator.
> > > > > ```
> > > > > By looking at `computeKnownBitsFromAssume()`, it doesn't look like that will affect the correctness of output,
> > > > > the bits that weren't inferred from assumption will simply remain unknown, correct?
> > > > That's right. computeKnownBitsFromAssume() ORs in any additional known bits it can determine. With the `computeKnownBits()` call it would start off from the `computeKnownBitsFromOperator()` bits, while with the direct call here it will start off with no known bits.
> > > Ok, so that is how i understood,
> > > That is sound to me, but i wonder if we can rely on that behavior.
> > > 
> > Wait, how do you even use `computeKnownBitsFromAssume()` here, it is a static function in `ValueTracking.cpp`?
> ... because we are in the `ValueTracking.cpp`, right, enough for today.
> But the other question remains.
> That is sound to me, but i wonder if we can rely on that behavior.

I think so. Or rather: I don't think it really matters either way. While this function will add in additional information, many other functions that accept KnownBits just overwrite it entirely. In this case both behaviors would get us the same result, as our starting state is "nothing known".


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59473