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

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Thanks, Mitch!
A couple of comments inline.


================
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.
----------------
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?

================
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);
----------------
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?

================
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.
----------------
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?


http://reviews.llvm.org/D12372