[X86] Fix sitofp and uitofp instruction matching failures with long double and avx512

[mbodart]

Summary

The operation action for i32 and i64 cannot be set to legal, as long double needs custom lowering.

[AsafBadouh]

lib/Target/X86/X86ISelLowering.cpp

1332 ↗

(On Diff #33220)

I'm not sure, but I think you missed SINT_TO_FP case.

[mbodart]

lib/Target/X86/X86ISelLowering.cpp

1332 ↗

(On Diff #33220)

Hi Asaf,

The SINT_TO_FP case does not require as many changes as the UINT_TO_FP case.

As the signed conversion instructions [v]cvtsi2ss/sd have existed since SSE/SSE2, LowerSINT_TO_FP already properly handles i32 and i64. And the operation action is properly set to Custom earlier in this function.

But the unsigned conversion instructions vcvtusi2ss/sd are new as of avx512f, so support for i32/i64->FP needed to be added to LowerUINT_TO_FP, and their operation action needed to be changed to Custom above.

regards,
Mitch

[mkuper]

Thanks, Mitch!
A couple of comments inline.

lib/Target/X86/X86ISelLowering.cpp

167 ↗

(On Diff #33220)

Any way to re-factor the if chain so we don't end up with two checks for useSoftFloat()? Or does that end up being uglier?

1332 ↗

(On Diff #33220)

I think what Asaf meant was that you removed setting SINT_TO_FP to Legal here, but didn't re-introduce it anywhere else. Does it get set to Legal elsewhere/by default?

12258 ↗

(On Diff #33220)

Do we only get here for f32/f64? If not, do you need to check the DstTy here is not f80? If yes, perhaps make the comment a bit clearer to emphasize that is the case?

[mbodart]

lib/Target/X86/X86ISelLowering.cpp

167 ↗

(On Diff #33220)

I think it would be about a wash. But there's a separate issue there, where UINT_TO_FP MVT::i64 is currently set to custom under is64Bit, independent of soft float. Retaining that behavior would make using a single soft float check a bit uglier, so is not something I want to mess with in this change set.

1332 ↗

(On Diff #33220)

It already gets set to Custom earlier in the routine.
The SINT setOperationAction is not present in the Phabricator view because
it's not near enough to the UINT changes.

For a conversion that has a mix of Legal and Custom lowerings
depending on the operand/result types, the legal cases are then handled
by letting Lower[US]INT_TO_FP be a no-op, just returning the original
operation unmodified. This was already happening correctly in the SINT case.

BTW, for additional testing I wrote small functions exercising all integer and fp
data types, compiled with llc across various triples and subtargets, and compared
the assembly output. The only changes were that the f80 cases under avx512f now
compile successfully (and produce reasonable code).

12258 ↗

(On Diff #33220)

f80 can reach here, but is filtered by the isScalarFPTypeInSSEReg check.
Though perhaps not obvious, this seems to be a commonly used check in this file
to distinguish between FP types in SSE registers (which must be f32 or f64), versus
FP types in X87 (like f80, but also f32/f64 under no-sse).

So it would be incomplete to single out just f80. I think the comment is
consistent with other uses of isScalarFPTypeInSSEReg in this file, so would
prefer to keep it as is.

[mkuper]

LGTM

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM,
 
   // Promote all UINT_TO_FP to larger SINT_TO_FP's, as X86 doesn't have this
   // operation.
   setOperationAction(ISD::UINT_TO_FP       , MVT::i1   , Promote);
   setOperationAction(ISD::UINT_TO_FP       , MVT::i8   , Promote);
   setOperationAction(ISD::UINT_TO_FP       , MVT::i16  , Promote);
 
   if (Subtarget->is64Bit()) {
-    setOperationAction(ISD::UINT_TO_FP     , MVT::i32  , Promote);
+    if (!Subtarget->useSoftFloat() && Subtarget->hasAVX512())
+      // f32/f64 are legal, f80 is custom.
+      setOperationAction(ISD::UINT_TO_FP   , MVT::i32  , Custom);
+    else
+      setOperationAction(ISD::UINT_TO_FP   , MVT::i32  , Promote);
     setOperationAction(ISD::UINT_TO_FP     , MVT::i64  , Custom);
   } else if (!Subtarget->useSoftFloat()) {
     // We have an algorithm for SSE2->double, and we turn this into a
     // 64-bit FILD followed by conditional FADD for other targets.
     setOperationAction(ISD::UINT_TO_FP     , MVT::i64  , Custom);
     // We have an algorithm for SSE2, and we turn this into a 64-bit
-    // FILD for other targets.
+    // FILD or VCVTUSI2SS/SD for other targets.
     setOperationAction(ISD::UINT_TO_FP     , MVT::i32  , Custom);
   }
 
   // Promote i1/i8 SINT_TO_FP to larger SINT_TO_FP's, as X86 doesn't have
   // this operation.
   setOperationAction(ISD::SINT_TO_FP       , MVT::i1   , Promote);
   setOperationAction(ISD::SINT_TO_FP       , MVT::i8   , Promote);
 
if (!Subtarget->useSoftFloat() && Subtarget->hasAVX512()) {
     setOperationAction(ISD::FSUB,               MVT::v8f64, Legal);
     setOperationAction(ISD::FMUL,               MVT::v8f64, Legal);
     setOperationAction(ISD::FDIV,               MVT::v8f64, Legal);
     setOperationAction(ISD::FSQRT,              MVT::v8f64, Legal);
     setOperationAction(ISD::FNEG,               MVT::v8f64, Custom);
     setOperationAction(ISD::FMA,                MVT::v8f64, Legal);
     setOperationAction(ISD::FMA,                MVT::v16f32, Legal);
 
-    // FIXME:  [US]INT_TO_FP are not legal for f80.
-    setOperationAction(ISD::SINT_TO_FP,         MVT::i32, Legal);
-    setOperationAction(ISD::UINT_TO_FP,         MVT::i32, Legal);
-    if (Subtarget->is64Bit()) {
-      setOperationAction(ISD::SINT_TO_FP,       MVT::i64, Legal);
-      setOperationAction(ISD::UINT_TO_FP,       MVT::i64, Legal);
-    }
     setOperationAction(ISD::FP_TO_SINT,         MVT::v16i32, Legal);
     setOperationAction(ISD::FP_TO_UINT,         MVT::v16i32, Legal);
     setOperationAction(ISD::FP_TO_UINT,         MVT::v8i32, Legal);
     setOperationAction(ISD::FP_TO_UINT,         MVT::v4i32, Legal);
     setOperationAction(ISD::SINT_TO_FP,         MVT::v16i32, Legal);
     setOperationAction(ISD::SINT_TO_FP,         MVT::v8i1,   Custom);
     setOperationAction(ISD::SINT_TO_FP,         MVT::v16i1,  Custom);
     setOperationAction(ISD::SINT_TO_FP,         MVT::v16i8,  Promote);
SDValue X86TargetLowering::LowerUINT_TO_FP(SDValue Op,
   // Since UINT_TO_FP is legal (it's marked custom), dag combiner won't
   // optimize it to a SINT_TO_FP when the sign bit is known zero. Perform
   // the optimization here.
   if (DAG.SignBitIsZero(N0))
     return DAG.getNode(ISD::SINT_TO_FP, dl, Op.getValueType(), N0);
 
   MVT SrcVT = N0.getSimpleValueType();
   MVT DstVT = Op.getSimpleValueType();
+
+  if (Subtarget->hasAVX512() && isScalarFPTypeInSSEReg(DstVT) &&
+      (SrcVT == MVT::i32 || (SrcVT == MVT::i64 && Subtarget->is64Bit()))) {
+    // Conversions from unsigned i32 to f32/f64 are legal,
+    // using VCVTUSI2SS/SD.  Same for i64 in 64-bit mode.
+    return Op;
+  }
+
   if (SrcVT == MVT::i64 && DstVT == MVT::f64 && X86ScalarSSEf64)
     return LowerUINT_TO_FP_i64(Op, DAG);
   if (SrcVT == MVT::i32 && X86ScalarSSEf64)
     return LowerUINT_TO_FP_i32(Op, DAG);
   if (Subtarget->is64Bit() && SrcVT == MVT::i64 && DstVT == MVT::f32)
     return SDValue();
 
   // Make a 64-bit buffer, and use it to build an FILD.

llvm/trunk/test/CodeGen/X86/scalar-int-to-fp.ll

+; Verify that scalar integer conversions to FP compile successfully
+; (at one time long double failed with avx512f), and that reasonable
+; instruction sequences are selected based on subtarget features.
+; Due to the plethora of reasonable sequences we just check for
+; one key instruction, usually a cvt or fild, allowing the test
+; to be relatively easily updated when sequences are improved.
+;
+; RUN: llc < %s -mtriple=i386-unknown-unknown     -mattr=+avx512f | FileCheck %s --check-prefix=CHECK --check-prefix=AVX512_32
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown   -mattr=+avx512f | FileCheck %s --check-prefix=CHECK --check-prefix=AVX512_64
+; RUN: llc < %s -mtriple=i386-unknown-unknown     -mattr=+sse2    | FileCheck %s --check-prefix=CHECK --check-prefix=SSE2_32
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown   -mattr=+sse2    | FileCheck %s --check-prefix=CHECK --check-prefix=SSE2_64
+; RUN: llc < %s -mtriple=i386-unknown-unknown     -mattr=-sse     | FileCheck %s --check-prefix=CHECK --check-prefix=X87
+
+; CHECK-LABEL: u32_to_f
+; AVX512_32: vcvtusi2ssl
+; AVX512_64: vcvtusi2ssl
+; SSE2_32: cvtsd2ss
+; SSE2_64: cvtsi2ssq
+; X87: fildll
+define float @u32_to_f(i32 %a) nounwind {
+  %r = uitofp i32 %a to float
+  ret float %r
+}
+
+; CHECK-LABEL: s32_to_f
+; AVX512_32: vcvtsi2ssl
+; AVX512_64: vcvtsi2ssl
+; SSE2_32: cvtsi2ssl
+; SSE2_64: cvtsi2ssl
+; X87: fildl
+define float @s32_to_f(i32 %a) nounwind {
+  %r = sitofp i32 %a to float
+  ret float %r
+}
+
+; CHECK-LABEL: u32_to_d
+; AVX512_32: vcvtusi2sdl
+; AVX512_64: vcvtusi2sdl
+; SSE2_32: subsd
+; SSE2_64: cvtsi2sdq
+; X87: fildll
+define double @u32_to_d(i32 %a) nounwind {
+  %r = uitofp i32 %a to double
+  ret double %r
+}
+
+; CHECK-LABEL: s32_to_d
+; AVX512_32: vcvtsi2sdl
+; AVX512_64: vcvtsi2sdl
+; SSE2_32: cvtsi2sdl
+; SSE2_64: cvtsi2sdl
+; X87: fildl
+define double @s32_to_d(i32 %a) nounwind {
+  %r = sitofp i32 %a to double
+  ret double %r
+}
+
+; CHECK-LABEL: u32_to_x
+; AVX512_32: vsubsd
+; AVX512_64: vsubsd
+; SSE2_32: subsd
+; SSE2_64: fildll
+; X87: fildll
+define x86_fp80 @u32_to_x(i32 %a) nounwind {
+  %r = uitofp i32 %a to x86_fp80
+  ret x86_fp80 %r
+}
+
+; CHECK-LABEL: s32_to_x
+; CHECK: fildl
+define x86_fp80 @s32_to_x(i32 %a) nounwind {
+  %r = sitofp i32 %a to x86_fp80
+  ret x86_fp80 %r
+}
+
+; CHECK-LABEL: u64_to_f
+; AVX512_32: fildll
+; AVX512_64: vcvtusi2ssq
+; SSE2_32: fildll
+; SSE2_64: cvtsi2ssq
+; X87: fildll
+define float @u64_to_f(i64 %a) nounwind {
+  %r = uitofp i64 %a to float
+  ret float %r
+}
+
+; CHECK-LABEL: s64_to_f
+; AVX512_32: fildll
+; AVX512_64: vcvtsi2ssq
+; SSE2_32: fildll
+; SSE2_64: cvtsi2ssq
+; X87: fildll
+define float @s64_to_f(i64 %a) nounwind {
+  %r = sitofp i64 %a to float
+  ret float %r
+}
+
+; CHECK-LABEL: u64_to_d
+; AVX512_32: vpunpckldq
+; AVX512_64: vcvtusi2sdq
+; SSE2_32: punpckldq
+; SSE2_64: punpckldq
+; X87: fildll
+define double @u64_to_d(i64 %a) nounwind {
+  %r = uitofp i64 %a to double
+  ret double %r
+}
+
+; CHECK-LABEL: s64_to_d
+; AVX512_32: fildll
+; AVX512_64: vcvtsi2sdq
+; SSE2_32: fildll
+; SSE2_64: cvtsi2sdq
+; X87: fildll
+define double @s64_to_d(i64 %a) nounwind {
+  %r = sitofp i64 %a to double
+  ret double %r
+}
+
+; CHECK-LABEL: u64_to_x
+; CHECK: fildll
+define x86_fp80 @u64_to_x(i64 %a) nounwind {
+  %r = uitofp i64 %a to x86_fp80
+  ret x86_fp80 %r
+}
+
+; CHECK-LABEL: s64_to_x
+; CHECK: fildll
+define x86_fp80 @s64_to_x(i64 %a) nounwind {
+  %r = sitofp i64 %a to x86_fp80
+  ret x86_fp80 %r
+}