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

[Evgenii Kudriashov via Phabricator via llvm-commits]

e-kud marked 6 inline comments as done.
e-kud added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30186
+        (Subtarget.hasSSE2() && VT == MVT::f64) ||
+        (Subtarget.hasFP16() && VT == MVT::f16)))
+    return SDValue();
----------------
RKSimon wrote:
> Again, you can drop this as the setOperationAction calls should ensure we never get here - replace it with an assertion if you're worried.
Yes, you are right. Everything must be handled in `X86TargetLowering`. Dropped these checks.


================
Comment at: llvm/test/CodeGen/X86/avx512fp16-fminimum-fmaximum.ll:153
+  ret half %2
+}
----------------
RKSimon wrote:
> please can you add vector test coverage to ensure we scalarize?
Yes, I've added them. Also it reminded me about several commented tests with the intrinsics. Uncommented them as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145634