[PATCH] D15946: Optimized instruction sequence for sitofp operation on X86-32

[Mitch Bodart via llvm-commits]

mbodart added a comment.

Hi Elena,

Just a few minor comments, otherwise LGTM!

- mitch


================
Comment at: ../lib/Target/X86/X86ISelLowering.cpp:12658
@@ +12657,3 @@
+      !Subtarget->is64Bit())
+    ValueToStore = DAG.getBitcast(MVT::f64, ValueToStore);
+
----------------
It would be useful to have a source comment here to the effect:

// 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.

================
Comment at: ../test/CodeGen/X86/scalar-int-to-fp.ll:76
@@ -75,3 +75,3 @@
 
 ; CHECK-LABEL: u64_to_f
 ; AVX512_32: fildll
----------------
For both test functions u64_to_f and s64_to_f, we should add the following additional checks
before the fildll:

AVX512_32: punpckldq
SSE_32: punpckldq

================
Comment at: ../test/CodeGen/X86/scalar-int-to-fp.ll:120-130
@@ -119,2 +119,13 @@
 
+; CHECK-LABEL: s64_to_d_2
+; SSE2_32: movd %ecx, %xmm0
+; SSE2_32: movd %eax, %xmm1
+; SSE2_32: punpckldq %xmm0, %xmm1
+; SSE2_32: fildll
+define double @s64_to_d_2(i64 %a) nounwind {
+  %b = add i64 %a, 5
+  %f = sitofp i64 %b to double
+  ret double %f
+}
+
 ; CHECK-LABEL: u64_to_x
----------------
Rather than creating a new function, it would seem more simple to just add a check for punpckldq, for both SSE2_32 and AVX512_32, in the existing s64_to_d function.


Repository:
  rL LLVM

http://reviews.llvm.org/D15946