[PATCH] D145634: [X86] Support llvm.{min,max}imum.f{16,32,64}

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30179
+  EVT VT = Op.getValueType();
+  if (Subtarget.useSoftFloat())
+    return SDValue();
----------------
You should be able to drop this early-out - we should never get here


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30186
+        (Subtarget.hasSSE2() && VT == MVT::f64) ||
+        (Subtarget.hasFP16() && VT == MVT::f16)))
+    return SDValue();
----------------
Again, you can drop this as the setOperationAction calls should ensure we never get here - replace it with an assertion if you're worried.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30224
+    Op = peekThroughBitcasts(Op);
+    if (ConstantFPSDNode *CstOp = dyn_cast<ConstantFPSDNode>(Op))
+      return CstOp->getValueAPF().bitcastToAPInt() == PreferredZero;
----------------
auto *


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30226
+      return CstOp->getValueAPF().bitcastToAPInt() == PreferredZero;
+    if (ConstantSDNode *CstOp = dyn_cast<ConstantSDNode>(Op))
+      return CstOp->getAPIntValue() == PreferredZero;
----------------
auto *


================
Comment at: llvm/test/CodeGen/X86/avx512fp16-fminimum-fmaximum.ll:153
+  ret half %2
+}
----------------
please can you add vector test coverage to ensure we scalarize?


================
Comment at: llvm/test/CodeGen/X86/fminimum-fmaximum.ll:962
+  ret float %2
+}
----------------
please can you add vector test coverage to ensure we scalarize?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145634