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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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);
----------------
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.


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

https://reviews.llvm.org/D137811