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

[Mitch Bodart via llvm-commits]

mbodart added inline comments.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:167
@@ -166,2 +166,3 @@
   if (Subtarget->is64Bit()) {
-    setOperationAction(ISD::UINT_TO_FP     , MVT::i32  , Promote);
+    if (!Subtarget->useSoftFloat() && Subtarget->hasAVX512())
+      // f32/f64 are legal, f80 is custom.
----------------
mkuper wrote:
> 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?
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.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:1332
@@ -1331,3 @@
-    if (Subtarget->is64Bit()) {
-      setOperationAction(ISD::SINT_TO_FP,       MVT::i64, Legal);
-      setOperationAction(ISD::UINT_TO_FP,       MVT::i64, Legal);
----------------
mkuper wrote:
> mbodart wrote:
> > AsafBadouh wrote:
> > > I'm not sure, but I think you missed SINT_TO_FP case.
> > 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
> 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?
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).

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:12258
@@ +12257,3 @@
+      (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.
----------------
mkuper wrote:
> 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?
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.


http://reviews.llvm.org/D12372