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

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added a comment.

Do you have plan to support `minimumNumber`?



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53237
+
+  if (!((Subtarget.hasSSE1() && VT == MVT::f32) ||
+        (Subtarget.hasSSE2() && VT == MVT::f64)))
----------------
`Subtarget.hasFP16() && VT == MVT::f16`


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53258
+
+  // We reuse FMAX and FMIN operations which are not commutative. They return
+  // the second operand if at least one of operands is NaN or if both operands
----------------
Better to give a table like above
```
  //                   Op1                        Op1      
  //               Num     xNaN                +0     -0  
  //            -----------------           --------------
  //       Num  |  Max  |  qNaN |       +0  |  +0  |  +0 |
  // Op0        -----------------  Op0      --------------
  //      xNaN  |  qNaN |  qNaN |       -0  |  +0  |  -0 |
  //            -----------------           --------------
```


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53284-53287
+    SDValue IsOp0Zero = DAG.getSetCC(DL, SetCCType,
+                                     DAG.getNode(ISD::BITCAST, DL, IVT, Op0),
+                                     DAG.getConstant(PreferredZero, DL, IVT),
+                                     ISD::SETEQ);
----------------
Can we check if it is 0 or NaN? We can put both in the second operand I think.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53215
 
+static SDValue combineFMinimumFMaximum(SDNode *N, SelectionDAG &DAG,
+                                       const X86Subtarget &Subtarget) {
----------------
e-kud wrote:
> pengfei wrote:
> > The code doesn't combine anything, should be moved to `LowerFMinimumFMaximum`?
> I've tried to find a better place to handle `ISD::FMAXIMUM` but there is no place in chain Selecting->Combining->Legalization. So, I was inspired by `ISD::FMAXNUM` that is actually lowered during combining.
> 
> If you are about naming solely, it is not a big deal to change the name, but all callees in `PerformDAGCombine` are named as `combine*` even for `ISD::FMAXNUM`. Do we really want to have a single `LowerFMinimumFMaximum`?
No, not the name. Did you try set action of `FMAXIMUM` to `Custom`? But I'm fine given it's similar to `combineFMinNumFMaxNum`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145634