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

[Evgenii Kudriashov via Phabricator via llvm-commits]

e-kud added a comment.

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

> In D145634#4194990 <https://reviews.llvm.org/D145634#4194990>, @e-kud wrote:
>
>> In D145634#4187914 <https://reviews.llvm.org/D145634#4187914>, @pengfei wrote:
>>
>>> In D145634#4187888 <https://reviews.llvm.org/D145634#4187888>, @e-kud wrote:
>>>
>>>> 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?
>>>
>>> It's accompanying function of `minimum` in IEEE-754 2019, it will be introduced in new C/C++ standard too. I thought you are working for that.
>>
>> I can't find any lib calls or specific intrinsics for them. I'd like to add them separately if we have any users or needs of them, do we?
>
> I heard glibc is supporting them. I'm fine with leaving it to the future. Do you have plan on the vector support?

Indeed, found this

> - <math.h> functions for floating-point maximum and minimum, corresponding to new operations in IEEE 754-2019, and corresponding <tgmath.h> macros, are added from draft ISO C2X: fmaximum, fmaximum_num, fmaximum_mag, fmaximum_mag_num, fminimum, fminimum_num, fminimum_mag, fminimum_mag_num and corresponding functions for float, long double, _FloatN and _FloatNx.

About vector support. I want to try to implement vector support alternatively, transform floats into integers using shifts and compare them as integers preserving float semantics, even -0 < +0. I've tried this approach for scalars but current approach produces less code. Probably vectors can benefit more.



================
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)) {
----------------
pengfei wrote:
> Need `hasFP16()` for `f16`.
Yes, thank you, missed it as `fp16` implies `dq`. We actually can check only `VT == MVT::f16` because above the predicate `Subtarget.hasFP16() && VT == MVT::f16` has been checked already. Do we want to avoid such implicit implication? Alternative is `VT != MVT::f16 && Subtarget.hasDQI() || VT == MVT::f16 && Subtarget.hasFP16()`.


================
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);
----------------
pengfei wrote:
> ditto format.
> Why do we need `getNode`?
Format doesn't work, we have 80+ chars if it's aligned with `return`. We don't need `getNode`, I missed it when moved from combine to lowering.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145634