[X86] Lower mathlib call ldexp into scalef when avx512 is enabled

[huhu233]

No description provided.

[huhu233]

Similar to #67552

[RKSimon]

Missing avx512 checks?

[huhu233]

Supplement the check, thanks!

[RKSimon]

no need to create a method - just make it static inside X86ISelLowering.cpp

[huhu233]

Done, thanks!

[RKSimon]

We'd be much better off adding a X86ISD::SCALEF nodetype rather than individual intrinsic lowering cases.

[huhu233]

Hi, @RKSimon, sorry for the delay in replying your comments, I have made some changes to the patch, please have a check, thanks very much!

[huhu233]

• Rebase the branch
• Use X86ISD::SCALEFS instead of specific intrinsics
• Transfrom the function into static version
• update the test file

[RKSimon]

Use the single line style like (most of) the previous cases.

[huhu233]

It seems that coding style check discourages this change ...

[RKSimon]

Why do you need to vectorize to use SCALEFS? I thought SCALEFS was for scalar types and SCALEF was for vector types? (So it should be possible to add vector support here as well).

[RKSimon]

Can we use vscalefph if we have AVX512FP16?

[phoebewang]

+1

[huhu233]

Hi, there may be risk of truncation, as the EXP operand of FLDEXP types i32, e.g., @llvm.ldexp.f16.i32(half, i32) ->@llvm.x86.avx512fp16.mask.scalef.sh(<8 x half>, <8 x half>, <8 x half>. I didn't know how to handle the issue elegantly, so I made an extension here.

[phoebewang]

Good point! LangRef doesn't give an example f16 case. Should we define it as @llvm.ldexp.f16.i16(half, i16)? i32 is a too large range to be useful for FP16.

[RKSimon]

Could we use value tracking to check the bounds of the EXP operand?

[huhu233]

Value tracking seems to make things very complicated.

[RKSimon]

I'd settle for a TODO comment for now

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff e34c35a21ccc215ce507a1e19b4ff2a1ce9906f3 4ac55f1aecf19b4e2ce981ad757c79b0daf46e5e -- llvm/lib/Target/X86/X86ISelLowering.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index 19510bbba0..7ac39cea21 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -2406,11 +2406,11 @@ X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM,
   }
 
   if (Subtarget.hasAVX512()) {
-    for (MVT VT : { MVT::f16, MVT::f32, MVT::f64, MVT::v4f32, MVT::v2f64 })
+    for (MVT VT : {MVT::f16, MVT::f32, MVT::f64, MVT::v4f32, MVT::v2f64})
       setOperationAction(ISD::FLDEXP, VT, Custom);
 
     if (Subtarget.hasVLX())
-      for (MVT VT : { MVT::v8f32, MVT::v4f64, MVT::v16f32, MVT::v8f64 })
+      for (MVT VT : {MVT::v8f32, MVT::v4f64, MVT::v16f32, MVT::v8f64})
         setOperationAction(ISD::FLDEXP, VT, Custom);
   }
 
@@ -32039,7 +32039,8 @@ SDValue X86TargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {
   case ISD::ADDRSPACECAST:      return LowerADDRSPACECAST(Op, DAG);
   case X86ISD::CVTPS2PH:        return LowerCVTPS2PH(Op, DAG);
   case ISD::PREFETCH:           return LowerPREFETCH(Op, Subtarget, DAG);
-  case ISD::FLDEXP:             return LowerFLDEXP(Op, Subtarget, DAG);
+  case ISD::FLDEXP:
+    return LowerFLDEXP(Op, Subtarget, DAG);
   }
 }
 

[huhu233]

• Fix coding style issues
• Support vector versions of ldexp, e.g., @llvm.ldexp.v8f32.v8i32 -- +avx512vl , @llvm.ldexp.v4f32.v4i32 -- +avx512f or +avx512vl, etc.

[huhu233]

• add TODO for avx512fp16

[RKSimon]

Are we sure this is right? I'd expect MVT::v4f32, MVT::v2f64, MVT::v8f32, MVT::v4f64 to be the VLX cases.

[arsenm]

Drop unnecessary fast flags?

[RKSimon]

@huhu233 reverse-ping

[Andarwinux]

Any progress on this PR?

[RKSimon]

Closing this abandoned PR and adding a issue (#165694) to track future work - I'll add the test coverage from this patch to trunk so we have something to work from.