[AMDGPU] Fix bitcast v4i64/v16i16

[piotr]

Summary

Fix a regression introduced in D128865.

[piotr]

While this fixes a regression in an important benchmark, I think we need to re-think how these BitConverts are specified in a follow-up work, as the current list has been highly inconsistent, even before my changes.

I guess, the list has grown organically from the cases that isel needs supporting. Does anyone know if we should be listing all possible cases of a bitcast for a given width? And always have both SReg/VReg? Or it's fine to have a list like specified currently?

[arsenm]

llvm/test/CodeGen/AMDGPU/bitcast-v4i64-v16i16.ll

30–31 ↗

(On Diff #443253)

Don't you need just this part and the other stores? Why the control flow?

[piotr]

llvm/test/CodeGen/AMDGPU/bitcast-v4i64-v16i16.ll

30–31 ↗

(On Diff #443253)

Looks the failing pattern is very sensitive to the IR coming to the isel. I couldn't repro this with only one bitcast either.

[arsenm]

llvm/test/CodeGen/AMDGPU/bitcast-v4i64-v16i16.ll

30–31 ↗

(On Diff #443253)

Merge with amdgcn.bitcast.ll?

I think the trick is to have the value defined in a different block and to not have it sink. You can reduce the block count

[piotr]

Good tip, thanks! Added a couple of tests in bitcast.ll, and reverted an erroneous edit from the original commit that the tests uncovered.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

setOperationAction(ISD::VECTOR_SHUFFLE,
                      Expand);
 
   setOperationAction(ISD::BUILD_VECTOR, {MVT::v4f16, MVT::v4i16}, Custom);
 
   // Avoid stack access for these.
   // TODO: Generalize to more vector types.
   setOperationAction({ISD::EXTRACT_VECTOR_ELT, ISD::INSERT_VECTOR_ELT},
                      {MVT::v2i16, MVT::v2f16, MVT::v2i8, MVT::v4i8, MVT::v8i8,
-                      MVT::v4i16, MVT::v4f16, MVT::v16i16, MVT::v16f16},
+                      MVT::v4i16, MVT::v4f16},
                      Custom);
 
   // Deal with vec3 vector operations when widened to vec4.
   setOperationAction(ISD::INSERT_SUBVECTOR,
                      {MVT::v3i32, MVT::v3f32, MVT::v4i32, MVT::v4f32}, Custom);
 
   // Deal with vec5/6/7 vector operations when widened to vec8.
   setOperationAction(ISD::INSERT_SUBVECTOR,

llvm/lib/Target/AMDGPU/SIInstructions.td

 def : BitConvert <v16f16, v8i32, VReg_256>;
 def : BitConvert <v16i16, v8i32, VReg_256>;
 def : BitConvert <v16f16, v8f32, VReg_256>;
 def : BitConvert <v16i16, v8f32, VReg_256>;
 def : BitConvert <v8i32, v16f16, VReg_256>;
 def : BitConvert <v8i32, v16i16, VReg_256>;
 def : BitConvert <v8f32, v16f16, VReg_256>;
 def : BitConvert <v8f32, v16i16, VReg_256>;
+def : BitConvert <v16f16, v4i64, VReg_256>;
+def : BitConvert <v16i16, v4i64, VReg_256>;
+def : BitConvert <v16f16, v4f64, VReg_256>;
+def : BitConvert <v16i16, v4f64, VReg_256>;
+def : BitConvert <v4i64, v16f16, VReg_256>;
+def : BitConvert <v4i64, v16i16, VReg_256>;
+def : BitConvert <v4f64, v16f16, VReg_256>;
+def : BitConvert <v4f64, v16i16, VReg_256>;
 
 // 512-bit bitcast
 def : BitConvert <v16i32, v16f32, VReg_512>;
 def : BitConvert <v16f32, v16i32, VReg_512>;
 def : BitConvert <v8i64,  v8f64,  VReg_512>;
 def : BitConvert <v8f64,  v8i64,  VReg_512>;
 def : BitConvert <v8i64,  v16i32, VReg_512>;
 def : BitConvert <v8f64,  v16i32, VReg_512>;

llvm/test/CodeGen/AMDGPU/amdgcn.bitcast.ll

 define amdgpu_kernel void @bitcast_f32_to_v1i32(i32 addrspace(1)* %out) {
   %f16 = call arcp afn half @llvm.canonicalize.f16(half 0xH03F0)
   %f32 = fpext half %f16 to float
   %v = bitcast float %f32 to <1 x i32>
   %v1 = extractelement <1 x i32> %v, i32 0
   store i32 %v1, i32 addrspace(1)* %out
   ret void
 }
+
+; FUNC-LABEL: {{^}}bitcast_v4i64_to_v16i16:
+define amdgpu_kernel void @bitcast_v4i64_to_v16i16(i32 %cond, <16 x i16> addrspace(1)* %out, <4 x i64> %value) {
+entry:
+  %cmp0 = icmp eq i32 %cond, 0
+  br i1 %cmp0, label %if, label %end
+
+if:
+  %phi_value = phi <4 x i64> [zeroinitializer, %entry], [%value, %if]
+  %cast = bitcast <4 x i64> %phi_value to <16 x i16>
+  %cmp1 = icmp eq i32 %cond, 1
+  br i1 %cmp1, label %if, label %end
+
+end:
+  %phi_cast = phi <16 x i16> [zeroinitializer, %entry], [%cast, %if]
+  store <16 x i16> %phi_cast, <16 x i16> addrspace(1)* %out
+  ret void
+}
+
+; FUNC-LABEL: {{^}}bitcast_v4f64_to_v16f16:
+define amdgpu_kernel void @bitcast_v4f64_to_v16f16(i32 %cond, <16 x half> addrspace(1)* %out, <4 x double> %value) {
+entry:
+  %cmp0 = icmp eq i32 %cond, 0
+  br i1 %cmp0, label %if, label %end
+
+if:
+  %phi_value = phi <4 x double> [zeroinitializer, %entry], [%value, %if]
+  %cast = bitcast <4 x double> %phi_value to <16 x half>
+  %cmp1 = icmp eq i32 %cond, 1
+  br i1 %cmp1, label %if, label %end
+
+end:
+  %phi_cast = phi <16 x half> [zeroinitializer, %entry], [%cast, %if]
+  store <16 x half> %phi_cast, <16 x half> addrspace(1)* %out
+  ret void
+}
+
+; FUNC-LABEL: {{^}}bitcast_v16i16_to_v4i64:
+define amdgpu_kernel void @bitcast_v16i16_to_v4i64(i32 %cond, <4 x i64> addrspace(1)* %out, <16 x i16> %value) {
+entry:
+  %cmp0 = icmp eq i32 %cond, 0
+  br i1 %cmp0, label %if, label %end
+
+if:
+  %phi_value = phi <16 x i16> [zeroinitializer, %entry], [%value, %if]
+  %cast = bitcast <16 x i16> %phi_value to <4 x i64>
+  %cmp1 = icmp eq i32 %cond, 1
+  br i1 %cmp1, label %if, label %end
+
+end:
+  %phi_cast = phi <4 x i64> [zeroinitializer, %entry], [%cast, %if]
+  store <4 x i64> %phi_cast, <4 x i64> addrspace(1)* %out
+  ret void
+}
+
+; FUNC-LABEL: {{^}}bitcast_v16f16_to_v4f64:
+define amdgpu_kernel void @bitcast_v16f16_to_v4f64(i32 %cond, <4 x double> addrspace(1)* %out, <16 x half> %value) {
+entry:
+  %cmp0 = icmp eq i32 %cond, 0
+  br i1 %cmp0, label %if, label %end
+
+if:
+  %phi_value = phi <16 x half> [zeroinitializer, %entry], [%value, %if]
+  %cast = bitcast <16 x half> %phi_value to <4 x double>
+  %cmp1 = icmp eq i32 %cond, 1
+  br i1 %cmp1, label %if, label %end
+
+end:
+  %phi_cast = phi <4 x double> [zeroinitializer, %entry], [%cast, %if]
+  store <4 x double> %phi_cast, <4 x double> addrspace(1)* %out
+  ret void
+}