[PATCH] D137811: InstCombine: Port amdgcn.class intrinsic combines to is.fpclass

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:6058-6066
+    // If all tests are made, it doesn't matter what the value is.
+    if ((Mask & fcAllFlags) == fcAllFlags)
+      return ConstantInt::get(ReturnType, true);
+    if ((Mask & fcAllFlags) == 0)
+      return ConstantInt::get(ReturnType, false);
+    if (isa<PoisonValue>(Op0))
+      return PoisonValue::get(ReturnType);
----------------
arsenm wrote:
> foad wrote:
> > arsenm wrote:
> > > foad wrote:
> > > > Poison/undef checks should probably go first, before other simplifications?
> > > I specifically had these later. The first operand shouldn't matter based on the test. I'm thinking about the interaction between fast math optimizations, and class uses used to guard regions where nnan/ninf is expected.
> > It's maybe not worth arguing about, but...
> > 
> > LangRef says "Most instructions return ‘poison’ when one of their arguments is ‘poison’". If you're saying is.fpclass is an exception to that rule then it at least ought to be documented. But I'm not sure why simplifying to poison would be a problem for the kind of code you're talking about - do you have an example?
> Cases like this, if you violate nnan/ninf but you have a test of the special cases:
> ```
> div = fdiv nan x / nan
> if (!is_fpclass(div, nan)) {
>     // do fast stuff
> }
> ```
> 
> 
> 
> I was viewing this as similar to select of poison. select with a poison condition propagates poison, but the value operands can be unobserved. Similarly here, a test of 0 shouldn't need to observe the actual compared value.
> 
> I was planning on revisiting this as I get further in optimizing all the special case checks in the math libraries. For now, this is the more conservative direction. The main thing I'm worried about is the asymmetry between equivalent fcmps and the class if we make this the expected behavior.
In the example you presented the operation producing NaN can be evaluated at compile time, so is_fpclass can be evaluated during compilation. There is no reason to create poison value here.

In the presence of nnan/ninf it is also saver to evaluate is_fpclass instead of poisoning it, because `fdiv nan, nan` and `is_fpclass(div, nan)` can come from different functions compiled with different fast-math settings merged, for example, by LTO. Poisoning can make a correct program non-working.


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

https://reviews.llvm.org/D137811