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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:234
+    else if (SignBit)
+      *SignBit &= *RHS.SignBit;
+
----------------
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 


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

https://reviews.llvm.org/D143195