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

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

LGTM modulo 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);
----------------
Poison/undef checks should probably go first, before other simplifications?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:846
+  if ((Mask & fcAllFlags) != Mask) {
+    CallInst *NewCall = Builder.CreateCall(
+      II.getCalledFunction(),
----------------
Might be simpler to modify the instruction in place with setOperand, and then Worklist.pushUsersToWorkList. But perhaps that is less clear.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:822
+
+  if (Mask == fcNan && !IsStrict) {
+    // Equivalent of isnan. Replace with standard fcmp.
----------------
foad wrote:
> Seems like you should handle `Mask == (fcAllFlags & ~fcNan)` here too. Same for the other `Mask ==` cases below. And allow InstCombine to "freely invert" this intrinsic by flipping all bits in the mask.
Please add a TODO for this if you're not going to do it now.


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

https://reviews.llvm.org/D137811