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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


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


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

https://reviews.llvm.org/D143195