Add memory variant of vcvtps2ph.

[alexr]

Also add the xmm version.

[RKSimon]

Thanks for working on this Alex - I've a concern with the 128->64 bit folded store.

lib/Target/X86/X86InstrSSE.td

8523

While VCVTPS2PHrr does write to an entire xmm (result half floats to lower 64-bits, and clears the upper 64-bits), VCVTPS2PHmr only writes out 64-bits to memory (so with this pattern the upper 64-bits would be undefined / not touched). Please can you update the pattern to use a suitable extract?

[alexr]

Try storing only 64 bits.

[alexr]

• Try storing only 64 bits.
• Support f64 as well as i64. Add more tests.

[RKSimon]

Thanks Alex, there is a 32-bit X86 test failure that needs dealing with but is good otherwise.

test/CodeGen/X86/f16c-intrinsics.ll

111

This fails on the X86 tests at the moment, but passes the X86-64 version fine.

[spatel]

Storing i64 on i386 with SSE2 is broken in general:
https://llvm.org/bugs/show_bug.cgi?id=23049

[spatel]

I'm almost afraid to ask, but why are these intrinsics defined with integer vectors?

def int_x86_vcvtps2ph_128 : GCCBuiltin<"__builtin_ia32_vcvtps2ph">,
            Intrinsic<[llvm_v8i16_ty], [llvm_v4f32_ty, llvm_i32_ty],
                      [IntrNoMem]>;

MIPS has v8f16 defs...so this should be possible for x86 too?

[spatel]

On Fri, Mar 27, 2015 at 5:40 PM, Ahmed Bougacha <[e-mail]> wrote:

On Fri, Mar 27, 2015 at 3:53 PM, Sanjay Patel <[e-mail]> wrote:
> I'm almost afraid to ask,

*can see where this is going*

> but why are these intrinsics defined with integer vectors?
>
>   def int_x86_vcvtps2ph_128 : GCCBuiltin<"__builtin_ia32_vcvtps2ph">,
>               Intrinsic<[llvm_v8i16_ty], [llvm_v4f32_ty, llvm_i32_ty],
>                         [IntrNoMem]>;
>
> MIPS has v8f16 defs...

Oh god, why?

> so this should be possible for x86 too?

It should, but would just be asking for trouble, with no benefit whatsoever.

With f16/half you're opening the door to any kind of half operation,
and you really don't want to do that (given that there's no such thing
except for these conversions, pretty much anywhere except GPUs).

On most targets, half is softened by the frontend to i16,  and this
avoids a whole class of problems.  If the ps2ph builtin returned <8 x
half>, I (or some optimization) could write:

  %1 = ..ps2ph..
  %2 = ..ps2ph..
  %l = fadd <8 x half> %1, %2

And this would just crash the compiler.
Also, clang emits __fp16 as i16, and it's reasonable to match that in
the vector builtins (though I'm not sure there's much interaction).

Ideally, we would always use f16/half, and let legalization take care
of it.  Until then, stick with integers ;)

Thanks, Ahmed! I don't know what MIPS is doing with v8f16, but I just took that as a grep-based existence proof. :)

I see now that X86ISelLowering has:

// TODO: when we have SSE, these could be more efficient, by using movd/movq.
if (!X86ScalarSSEf64) {
  setOperationAction(ISD::BITCAST        , MVT::f32  , Expand);
  ...

I haven't looked into how that part works before, but I'm guessing that's where I need to hack to get i64 stores working for i386 in general...and that'll solve this problem with f16 intrinsics too.

[spatel]

Let the yak shaving begin: D8691 is the first of hopefully not too many steps to fix this problem.

[spatel]

This fixes the 64-bit store on x86-32:
http://reviews.llvm.org/rL235460

So unless there are any other objections, this patch should be good to go.

[RKSimon]

With the -mtriple fix to the test case LGTM

test/CodeGen/X86/f16c-intrinsics.ll

3

Please can you replace -march with a -mtriple equivalent - it prevents the tests failing on windows build/tests

lib/Target/X86/X86InstrSSE.td

let Predicates = [HasF16C] in {
   defm VCVTPS2PH  : f16c_ps2ph<VR128, f64mem, int_x86_vcvtps2ph_128>;
   defm VCVTPS2PHY : f16c_ps2ph<VR256, f128mem, int_x86_vcvtps2ph_256>, VEX_L;
 
   // Pattern match vcvtph2ps of a scalar i64 load.
   def : Pat<(int_x86_vcvtph2ps_128 (vzmovl_v2i64 addr:$src)),
             (VCVTPH2PSrm addr:$src)>;
   def : Pat<(int_x86_vcvtph2ps_128 (vzload_v2i64 addr:$src)),
             (VCVTPH2PSrm addr:$src)>;
+            
+  def : Pat<(store (v8i16 (int_x86_vcvtps2ph_128 VR128:$src1, i32:$src2)),
+                   addr:$dst),
+                   (VCVTPS2PHmr addr:$dst, VR128:$src1, imm:$src2)>;
+  def : Pat<(store (v8i16 (int_x86_vcvtps2ph_256 VR256:$src1, i32:$src2)),
+                   addr:$dst),
+                   (VCVTPS2PHYmr addr:$dst, VR256:$src1, imm:$src2)>;
 }
 
 // Patterns for  matching conversions from float to half-float and vice versa.
 let Predicates = [HasF16C] in {
   def : Pat<(fp_to_f16 FR32:$src),
             (i16 (EXTRACT_SUBREG (VMOVPDI2DIrr (VCVTPS2PHrr
               (COPY_TO_REGCLASS FR32:$src, VR128), 0)), sub_16bit))>;
 

test/CodeGen/X86/f16c-intrinsics.ll

 ; RUN: llc < %s -march=x86 -mattr=+avx,+f16c | FileCheck %s
 ; RUN: llc < %s -march=x86-64 -mattr=+avx,+f16c | FileCheck %s
 
 define <4 x float> @test_x86_vcvtph2ps_128(<8 x i16> %a0) {
   ; CHECK-LABEL: test_x86_vcvtph2ps_128
   ; CHECK-NOT: vmov
   ; CHECK: vcvtph2ps
   %res = call <4 x float> @llvm.x86.vcvtph2ps.128(<8 x i16> %a0) ; <<4 x float>> [#uses=1]
   ret <4 x float> %res
 }
 declare <4 x float> @llvm.x86.vcvtph2ps.128(<8 x i16>) nounwind readonly
define <8 x i16> @test_x86_vcvtps2ph_128(<4 x float> %a0) {
   ; CHECK-LABEL: test_x86_vcvtps2ph_128
   ; CHECK-NOT: vmov
   ; CHECK: vcvtps2ph
   %res = call <8 x i16> @llvm.x86.vcvtps2ph.128(<4 x float> %a0, i32 0) ; <<8 x i16>> [#uses=1]
   ret <8 x i16> %res
 }
 declare <8 x i16> @llvm.x86.vcvtps2ph.128(<4 x float>, i32) nounwind readonly
 
+define void @test_x86_vcvtps2ph_128_m(<8 x i16>* nocapture %d, <4 x float> %a) nounwind {
+entry:
+  ; CHECK-LABEL: test_x86_vcvtps2ph_128_m:
+  ; CHECK-NOT: vmov
+  ; CHECK: vcvtps2ph  $3, %xmm0, (%
+  %0 = tail call <8 x i16> @llvm.x86.vcvtps2ph.128(<4 x float> %a, i32 3)
+  store <8 x i16> %0, <8 x i16>* %d, align 16
+  ret void
+}
 
 define <8 x i16> @test_x86_vcvtps2ph_256(<8 x float> %a0) {
   ; CHECK-LABEL: test_x86_vcvtps2ph_256
   ; CHECK-NOT: vmov
   ; CHECK: vcvtps2ph
   %res = call <8 x i16> @llvm.x86.vcvtps2ph.256(<8 x float> %a0, i32 0) ; <<8 x i16>> [#uses=1]
   ret <8 x i16> %res
 }
 declare <8 x i16> @llvm.x86.vcvtps2ph.256(<8 x float>, i32) nounwind readonly
 
 define <4 x float> @test_x86_vcvtps2ph_128_scalar(i64* %ptr) {
 ; CHECK-LABEL: test_x86_vcvtps2ph_128_scalar
 ; CHECK-NOT: vmov
 ; CHECK: vcvtph2ps (%
 
   %load = load i64* %ptr
   %ins1 = insertelement <2 x i64> undef, i64 %load, i32 0
   %ins2 = insertelement <2 x i64> %ins1, i64 0, i32 1
   %bc = bitcast <2 x i64> %ins2 to <8 x i16>
   %res = tail call <4 x float> @llvm.x86.vcvtph2ps.128(<8 x i16> %bc) #2
   ret <4 x float> %res
 }
+
+define void @test_x86_vcvtps2ph_256_m(<8 x i16>* nocapture %d, <8 x float> %a) nounwind {
+entry:
+  ; CHECK-LABEL: test_x86_vcvtps2ph_256_m:
+  ; CHECK-NOT: vmov
+  ; CHECK: vcvtps2ph  $3, %ymm0, (%
+  %0 = tail call <8 x i16> @llvm.x86.vcvtps2ph.256(<8 x float> %a, i32 3)
+  store <8 x i16> %0, <8 x i16>* %d, align 16
+  ret void
+}