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

[Evgenii Kudriashov via Phabricator via llvm-commits]

e-kud marked an inline comment as done.
e-kud added a comment.

In D145634#4183847 <https://reviews.llvm.org/D145634#4183847>, @pengfei wrote:

> Do you have plan to support `minimumNumber`?

Sorry, I didn't get it. Could you be more specific?



================
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);
----------------
pengfei wrote:
> pengfei wrote:
> > Can we check if it is 0 or NaN? We can put both in the second operand I think.
> We can use `vfpclassps/d` on `AVX512DQ` to optimize it.
It was my first attempt to compare with 0 and NaN at the same time. We have two problems. The first is that comparison with zero returns ZF regardless positive or negative zero is provided. The second is that we still need to know whether the second operand is NaN or not. We may have `(0.0, NaN)` as arguments. We checked that the first op is not NaN and is zero. Replaced it with the second when the second is NaN. It seems to me that we always need to check both operands on NaN and one check on zero.

We can use `vfpclassps/d` on `AVX512DQ` if one of operands is known never NaN. Working on it.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53215
 
+static SDValue combineFMinimumFMaximum(SDNode *N, SelectionDAG &DAG,
+                                       const X86Subtarget &Subtarget) {
----------------
pengfei wrote:
> pengfei wrote:
> > 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`.
> Answer my previous question: `combineFMinNumFMaxNum` was intended here due to the reason described in D15294. I suppose the problem doesn't exist to `combineFMinimumFMaximum`. So I prefer to `LowerFMinimumFMaximum`.
Yes, thank you. I was unaware of this mechanism. It works but I've needed to include extra logic because with `Custom` lowering there is no more `setcc` combining.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145634