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

[Joshua Cranmer via Phabricator via llvm-commits]

jcranmer-intel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:819
+  const ConstantInt *CMask = cast<ConstantInt>(Src1);
+  uint32_t Mask = CMask->getZExtValue();
+  const bool IsStrict = II.isStrictFP();
----------------
It feels like you should be able to use ninf/nnan/nsz flags to modify the mask here to make it more likely to fall into one of these categories, although this may provoke some backlash from those who argue that `nnan isnan(x)` => unconditional `false` shouldn't be an allowable optimization.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:837
+  }
+
+  // fp_class (nnan x), qnan|snan|other -> fp_class (nnan x), other
----------------
You're also missing cases for `Mask == fcPosInf` and `Mask == fcNegInf`, which can similarly be lowered to quiet comparisons.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:842
+    // Equivalent of isnan. Replace with standard fcmp.
+    Value *FCmp = Builder.CreateFCmpUNO(Src0, Src0);
+    FCmp->takeName(&II);
----------------
sepavloff wrote:
> arsenm wrote:
> > sepavloff wrote:
> > > This replacement is not valid in general case, only if FP exceptions are ignored. If the argument is a signaling NaN, compare instruction raises `Invalid` exception.
> > Isn't that implied by not being a constrained intrinsic?
> > 
> `is_fpclass` does not depend on FP environment and does not change it. So it does not have constrained variant.
`Builder.CreateFCmp*` functions look like they create quiet comparison instructions in FP-constrained mode, and you need to use `CreateFCmpS` to generate one of the signalling comparisons. So replacing them even in strict mode is legal.


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

https://reviews.llvm.org/D137811