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

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:1004-1005
                                                     : &X86::VR128RegClass);
+    setOperationAction(ISD::FMAXIMUM, MVT::f32, Custom);
+    setOperationAction(ISD::FMINIMUM, MVT::f32, Custom);
 
----------------
Make the format align with its context.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:2132-2133
     setOperationAction(ISD::STRICT_FP_ROUND,      MVT::f16, Custom);
+    setOperationAction(ISD::FMAXIMUM, MVT::f16, Custom);
+    setOperationAction(ISD::FMINIMUM, MVT::f16, Custom);
     setOperationAction(ISD::FP_EXTEND,            MVT::f32, Legal);
----------------
ditto.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:29902
+  //            -------------------           -----------------
+  //       Num  | Max/Min |  qNaN |       +0  |  +0   | +0/-0 |
+  // Op0        -------------------  Op0      -----------------
----------------
Put Max/Min together is a bit confusing. My first impression is it can return either +0 or -0 for a single comparison.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:29931
+    MinMax = DAG.getNode(MinMaxOp, DL, VT, Op1, Op0, N->getFlags());
+  } else if (Subtarget.hasDQI() &&
+             (N->getFlags().hasNoNaNs() || IsOp0NeverNaN || IsOp1NeverNaN)) {
----------------
Need `hasFP16()` for `f16`.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:33715
+  case ISD::FMAXIMUM:
+    return LowerFMinimumFMaximum(Op.getNode(), DAG, Subtarget);
   case ISD::ABS:                return LowerABS(Op, Subtarget, DAG);
----------------
ditto format.
Why do we need `getNode`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145634