[X86] Remove unnecessary bitcasting in INT_TO_FP

[icedrocket]

Summary

This seems to have been added to prevent two 32-bit stores in older versions. But the latest version doesn't seem to use two 32-bit stores for casting. This increases the stack size and causes unnecessary bitcasting. Do we still need this?

[craig.topper]

Please update the tests in llvm/test/CodeGen/X86 along with the patch so reviewers can make decision.

[craig.topper]

I applied the patch myself and checked the tests. After this change we do 2 stores as the comment says we would

See comments below

diff --git a/llvm/test/CodeGen/X86/uint64-to-float.ll b/llvm/test/CodeGen/X86/uint64-to-float.ll
index 8b6623476eba..b1de2b7ec005 100644
--- a/llvm/test/CodeGen/X86/uint64-to-float.ll
+++ b/llvm/test/CodeGen/X86/uint64-to-float.ll
@@ -13,12 +13,13 @@ define float @test(i64 %a) nounwind {
 ; X86-NEXT:    movl %esp, %ebp
 ; X86-NEXT:    andl $-8, %esp
 ; X86-NEXT:    subl $16, %esp
-; X86-NEXT:    movl 12(%ebp), %eax
-; X86-NEXT:    movsd {{.*#+}} xmm0 = mem[0],zero
-; X86-NEXT:    movlps %xmm0, {{[0-9]+}}(%esp).    <- single f64 store
-; X86-NEXT:    shrl $31, %eax
+; X86-NEXT:    movl 8(%ebp), %eax
+; X86-NEXT:    movl 12(%ebp), %ecx
+; X86-NEXT:    movl %ecx, {{[0-9]+}}(%esp).  <- store 1
+; X86-NEXT:    movl %eax, {{[0-9]+}}(%esp)   <- store 2
+; X86-NEXT:    shrl $31, %ecx
 ; X86-NEXT:    fildll {{[0-9]+}}(%esp)
-; X86-NEXT:    fadds {{\.?LCPI[0-9]+_[0-9]+}}(,%eax,4)
+; X86-NEXT:    fadds {{\.?LCPI[0-9]+_[0-9]+}}(,%ecx,4)
 ; X86-NEXT:    fstps {{[0-9]+}}(%esp)
 ; X86-NEXT:    movss {{.*#+}} xmm0 = mem[0],zero,zero,zero
 ; X86-NEXT:    movss %xmm0, (%esp)

[icedrocket]

You're right, I should have checked enough before request a review.

Sorry for confusion.

llvm/lib/Target/X86/X86ISelLowering.cpp

if (IsStrict)
                          {Chain, Ext});
 
     return DAG.getNode(ISD::SINT_TO_FP, dl, VT, Ext);
   }
 
   if (VT == MVT::f128 || !Subtarget.hasX87())
     return SDValue();
 
-  SDValue ValueToStore = Src;
-  if (SrcVT == MVT::i64 && Subtarget.hasSSE2() && !Subtarget.is64Bit())
-    // Bitcasting to f64 here allows us to do a single 64-bit store from
-    // an SSE register, avoiding the store forwarding penalty that would come
-    // with two 32-bit stores.
-    ValueToStore = DAG.getBitcast(MVT::f64, ValueToStore);
-
   unsigned Size = SrcVT.getStoreSize();
   Align Alignment(Size);
   MachineFunction &MF = DAG.getMachineFunction();
   auto PtrVT = getPointerTy(MF.getDataLayout());
   int SSFI = MF.getFrameInfo().CreateStackObject(Size, Alignment, false);
   MachinePointerInfo MPI =
       MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), SSFI);
   SDValue StackSlot = DAG.getFrameIndex(SSFI, PtrVT);
-  Chain = DAG.getStore(Chain, dl, ValueToStore, StackSlot, MPI, Alignment);
+  Chain = DAG.getStore(Chain, dl, Src, StackSlot, MPI, Alignment);
   std::pair<SDValue, SDValue> Tmp =
       BuildFILD(VT, SrcVT, dl, Chain, StackSlot, MPI, Alignment, DAG);
 
   if (IsStrict)
     return DAG.getMergeValues({Tmp.first, Tmp.second}, dl);
 
   return Tmp.first;
 }
std::pair<SDValue, SDValue> Tmp =
         BuildFILD(DstVT, MVT::i64, dl, Store2, StackSlot, MPI, SlotAlign, DAG);
     if (IsStrict)
       return DAG.getMergeValues({Tmp.first, Tmp.second}, dl);
 
     return Tmp.first;
   }
 
   assert(SrcVT == MVT::i64 && "Unexpected type in UINT_TO_FP");
-  SDValue ValueToStore = Src;
-  if (isScalarFPTypeInSSEReg(Op.getValueType()) && !Subtarget.is64Bit()) {
-    // Bitcasting to f64 here allows us to do a single 64-bit store from
-    // an SSE register, avoiding the store forwarding penalty that would come
-    // with two 32-bit stores.
-    ValueToStore = DAG.getBitcast(MVT::f64, ValueToStore);
-  }
-  SDValue Store =
-      DAG.getStore(Chain, dl, ValueToStore, StackSlot, MPI, SlotAlign);
+
+  SDValue Store = DAG.getStore(Chain, dl, Src, StackSlot, MPI, SlotAlign);
   // For i64 source, we need to add the appropriate power of 2 if the input
   // was negative. We must be careful to do the computation in x87 extended
   // precision, not in SSE.
   SDVTList Tys = DAG.getVTList(MVT::f80, MVT::Other);
   SDValue Ops[] = { Store, StackSlot };
   SDValue Fild =
       DAG.getMemIntrinsicNode(X86ISD::FILD, dl, Tys, Ops, MVT::i64, MPI,
                               SlotAlign, MachineMemOperand::MOLoad);
   Chain = Fild.getValue(1);
 
-
   // Check whether the sign bit is set.
   SDValue SignSet = DAG.getSetCC(
       dl, getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(), MVT::i64),
       Op.getOperand(OpNo), DAG.getConstant(0, dl, MVT::i64), ISD::SETLT);
 
   // Build a 64 bit pair (FF, 0) in the constant pool, with FF in the hi bits.
   APInt FF(64, 0x5F80000000000000ULL);
   SDValue FudgePtr = DAG.getConstantPool(