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

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:847
+  // fp_class (nnan x), qnan|snan|other -> fp_class (nnan x), other
+  if ((Mask & fcNan) && isKnownNeverNaN(Src0, &getTargetLibraryInfo())) {
+    return replaceOperand(II, 1,
----------------
These removal of Mask bits should come before the `Mask == $X` tests, shouldn't they?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:856
+  // Clamp mask to used bits
+  if ((Mask & fcAllFlags) != Mask) {
+    CallInst *NewCall = Builder.CreateCall(
----------------
Why are unknown bits even accepted? ISTM it should be an error in Verifier::visitIntrinsicCall to pass invalid bits.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:824
+    // Equivalent of isnan. Replace with standard fcmp.
+    Value *FCmp = Builder.CreateFCmpUNO(Src0, Src0);
+    FCmp->takeName(&II);
----------------
arsenm wrote:
> sepavloff wrote:
> > Is it profitable to make such replacement early? Is there any advantage, at least hypothetical? 
> I think an fcmp should be more canonical and better handled by existing optimizations, than class which is handled ~nowhere
This seems OK.

I'm not sure it's the //best// choice -- if a CPU actually has an fpclassify instruction, is it really a good idea to canonicalize in generic code to fcmp? But I think that's fine to revisit later if it becomes a problem.


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

https://reviews.llvm.org/D137811