[PATCH] D143195: ValueTracking: Add start of computeKnownFPClass API

[Joshua Cranmer via Phabricator via llvm-commits]

jcranmer-intel added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:234
+    else if (SignBit)
+      *SignBit &= *RHS.SignBit;
+
----------------
arsenm wrote:
> nikic wrote:
> > arsenm wrote:
> > > nikic wrote:
> > > > I don't understand your logic here. If `&` is supposed to be (doc comments?) an intersection, then if one side knows the sign bit and the other doesn't, shouldn't we be using the known sign bit?
> > > > 
> > > > It doesn't look like `operator&=` or `commonBits()` is actually used though. It may be worthwhile to split out this class into a separate header (like KnownBits) and unit test it.
> > > If you don't know one of the sign bits you don't know anything. & should only return true if we know both sign bits must be true
> > Isn't that inconsistent with what you do with the FP classes? Say the left one is fcNegative | fcPositive with SignBit unknown and the right one is fcPositive with SignBit one. Now I would expect `&` to return fcPositive with SignBit one, but you return fcPositive with SignBit unknown.
> > 
> > (I'm assuming you aren't treating `&` as a literally a bitwise operation on the bitwise float representation, because then the FP class handling makes no sense.)
> There are additional fixups for the known sign bit implied by the mask, like your example. This should be conservatively correct as-is. But since it's unused I can just drop this and sort it out later 
I'm with nikic here--the natural interpretation of &/| for this datastructure is intersection/union of knowledge, and the sign bit handling in & seems out-of-place compared to in other places. Without seeing uses of this method, it's not clear to me why the unknown-if-either-is-unknown should be correct for &.


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

https://reviews.llvm.org/D143195