[PATCH] D145634: [X86] Support llvm.{min,max}imum.f{32,64}
Phoebe Wang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 9 23:04:21 PST 2023
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
More information about the llvm-commits
mailing list