[PATCH] D108470: [OpenCL] Fix as_type3 invalid store creation

[Sven van Haastregt via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4da744a20ff5: [OpenCL] Fix as_type3 invalid store creation 
(authored by svenvh).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108470/new/

https://reviews.llvm.org/D108470

Files:
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/CodeGenOpenCL/preserve_vec3.cl


Index: clang/test/CodeGenOpenCL/preserve_vec3.cl
===
--- clang/test/CodeGenOpenCL/preserve_vec3.cl
+++ clang/test/CodeGenOpenCL/preserve_vec3.cl
@@ -1,6 +1,7 @@
 // RUN: %clang_cc1 %s -emit-llvm -o - -triple spir-unknown-unknown 
-fpreserve-vec3-type | FileCheck %s
 
 typedef char char3 __attribute__((ext_vector_type(3)));
+typedef char char8 __attribute__((ext_vector_type(8)));
 typedef short short3 __attribute__((ext_vector_type(3)));
 typedef double double2 __attribute__((ext_vector_type(2)));
 typedef float float3 __attribute__((ext_vector_type(3)));
@@ -38,6 +39,15 @@
   *b = __builtin_astype(*a, double2);
 }
 
+void kernel char8_to_short3(global short3 *a, global char8 *b) {
+  // CHECK-LABEL: spir_kernel void @char8_to_short3
+  // CHECK: %[[IN_BC:.*]] = bitcast <8 x i8> addrspace(1)* %b to <4 x i16> 
addrspace(1)*
+  // CHECK: %[[LOAD_B:.*]] = load <4 x i16>, <4 x i16> addrspace(1)* %[[IN_BC]]
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x i16> %[[LOAD_B]], <4 x i16> 
poison, <3 x i32> 
+  // CHECK: store <3 x i16> %[[ASTYPE]], <3 x i16> addrspace(1)* %a, align 8
+  *a = __builtin_astype(*b, short3);
+}
+
 void from_char3(char3 a, global int *out) {
   // CHECK-LABEL: void @from_char3
   // CHECK: %[[ASTYPE:.*]] = shufflevector <3 x i8> %a, <3 x i8> poison, <4 x 
i32> 
@@ -53,3 +63,19 @@
   // CHECK: store <4 x i16> %[[ASTYPE]], <4 x i16> addrspace(1)* %[[OUT_BC]]
   *out = __builtin_astype(a, long);
 }
+
+void scalar_to_char3(int a, global char3 *out) {
+  // CHECK-LABEL: void @scalar_to_char3
+  // CHECK: %[[IN_BC:.*]] = bitcast i32 %a to <4 x i8>
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x i8> %[[IN_BC]], <4 x i8> 
poison, <3 x i32> 
+  // CHECK: store <3 x i8> %[[ASTYPE]], <3 x i8> addrspace(1)* %out
+  *out = __builtin_astype(a, char3);
+}
+
+void scalar_to_short3(long a, global short3 *out) {
+  // CHECK-LABEL: void @scalar_to_short3
+  // CHECK: %[[IN_BC:.*]] = bitcast i64 %a to <4 x i16>
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x i16> %[[IN_BC]], <4 x i16> 
poison, <3 x i32> 
+  // CHECK: store <3 x i16> %[[ASTYPE]], <3 x i16> addrspace(1)* %out
+  *out = __builtin_astype(a, short3);
+}
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -4804,12 +4804,10 @@
   // to vec4 if the original type is not vec4, then a shuffle vector to
   // get a vec3.
   if (NumElementsSrc != 3 && NumElementsDst == 3) {
-if (!CGF.CGM.getCodeGenOpts().PreserveVec3Type) {
-  auto *Vec4Ty = llvm::FixedVectorType::get(
-  cast(DstTy)->getElementType(), 4);
-  Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(), Src,
- Vec4Ty);
-}
+auto *Vec4Ty = llvm::FixedVectorType::get(
+cast(DstTy)->getElementType(), 4);
+Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(), Src,
+   Vec4Ty);
 
 Src = ConvertVec3AndVec4(Builder, CGF, Src, 3);
 Src->setName("astype");


Index: clang/test/CodeGenOpenCL/preserve_vec3.cl
===
--- clang/test/CodeGenOpenCL/preserve_vec3.cl
+++ clang/test/CodeGenOpenCL/preserve_vec3.cl
@@ -1,6 +1,7 @@
 // RUN: %clang_cc1 %s -emit-llvm -o - -triple spir-unknown-unknown -fpreserve-vec3-type | FileCheck %s
 
 typedef char char3 __attribute__((ext_vector_type(3)));
+typedef char char8 __attribute__((ext_vector_type(8)));
 typedef short short3 __attribute__((ext_vector_type(3)));
 typedef double double2 __attribute__((ext_vector_type(2)));
 typedef float float3 __attribute__((ext_vector_type(3)));
@@ -38,6 +39,15 @@
   *b = __builtin_astype(*a, double2);
 }
 
+void kernel char8_to_short3(global short3 *a, global char8 *b) {
+  // CHECK-LABEL: spir_kernel void @char8_to_short3
+  // CHECK: %[[IN_BC:.*]] = bitcast <8 x i8> addrspace(1)* %b to <4 x i16> addrspace(1)*
+  // CHECK: %[[LOAD_B:.*]] = load <4 x i16>, <4 x i16> addrspace(1)* %[[IN_BC]]
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x i16> %[[LOAD_B]], <4 x i16> poison, <3 x i32> 
+  // CHECK: store <3 x i16> %[[ASTYPE]], <3 x i16> addrspace(1)* %a, align 8
+  *a = __builtin_astype(*b, short3);
+}
+
 void from_char3(char3 a, global int *out) {
   // CHECK-LABEL: void @from_char3
   // CHECK: %[[ASTYPE:.*]] = shufflevector <3 x i8> %a, <3 x i8> poison, <4 x i32> 
@@ -53,3 +63,19 @@
   // CHECK: store <4 x i16> %[[ASTYPE]], <4 

[PATCH] D108470: [OpenCL] Fix as_type3 invalid store creation

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.

LGTM! Thanks


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108470/new/

https://reviews.llvm.org/D108470

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108470: [OpenCL] Fix as_type3 invalid store creation

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added a comment.

>> Instead of `double2_to_float3`, I decided to add `char8_to_short3`.  Same 
>> idea (vectorN to vector3), but reinterpreting 8 chars as 3 shorts feels like 
>> a more realistic case than reinterpreting 2 doubles as 3 floats.  But I'm 
>> happy to add `double2_to_float3` if you think there is a good reason to do 
>> so.
>
> I am sorry for asking more tests because I did not add enough tests 
> previously.
> I do not know well how many type conversions with vec3 the recent OpenCL spec 
> cover but it could be good to add more the tests of the type conversions 
> which the spec mentions. If you feel the tests you have added are enough for 
> it, I am ok with it.

You were absolutely right about asking for more tests!  I believe we now have 
much better coverage, let's see if @Anastasia (or any other reviewer) is also 
fine with the current set.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108470/new/

https://reviews.llvm.org/D108470

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108470: [OpenCL] Fix as_type3 invalid store creation

[JinGu Kang via Phabricator via cfe-commits]

jaykang10 added a comment.

In D108470#2959715 , @svenvh wrote:

> In D108470#2959708 , @jaykang10 
> wrote:
>
>> Can you also add "double2_to_float3"
>
> Instead of `double2_to_float3`, I decided to add `char8_to_short3`.  Same 
> idea (vectorN to vector3), but reinterpreting 8 chars as 3 shorts feels like 
> a more realistic case than reinterpreting 2 doubles as 3 floats.  But I'm 
> happy to add `double2_to_float3` if you think there is a good reason to do so.

I am sorry for asking more tests because I did not add enough tests previously.
I do not know well how many type conversions with vec3 the recent OpenCL spec 
cover but it could be good to add more the tests of the type conversions which 
the spec mentions. If you feel the tests you have added are enough for it, I am 
ok with it.

>> and "float4_to_float3" tests please?
>
> That test is already there, see line 17.

Sorry, I missed it.

LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108470/new/

https://reviews.llvm.org/D108470

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108470: [OpenCL] Fix as_type3 invalid store creation

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added a comment.

In D108470#2959708 , @jaykang10 wrote:

> Can you also add "double2_to_float3"

Instead of `double2_to_float3`, I decided to add `char8_to_short3`.  Same idea 
(vectorN to vector3), but reinterpreting 8 chars as 3 shorts feels like a more 
realistic case than reinterpreting 2 doubles as 3 floats.  But I'm happy to add 
`double2_to_float3` if you think there is a good reason to do so.

> and "float4_to_float3" tests please?

That test is already there, see line 17.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108470/new/

https://reviews.llvm.org/D108470

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108470: [OpenCL] Fix as_type3 invalid store creation

[JinGu Kang via Phabricator via cfe-commits]

jaykang10 added a comment.

Can you also add "double2_to_float3" and "float4_to_float3" tests please?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108470/new/

https://reviews.llvm.org/D108470

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108470: [OpenCL] Fix as_type3 invalid store creation

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh updated this revision to Diff 368058.
svenvh added a comment.

Added more tests as requested.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108470/new/

https://reviews.llvm.org/D108470

Files:
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/CodeGenOpenCL/preserve_vec3.cl


Index: clang/test/CodeGenOpenCL/preserve_vec3.cl
===
--- clang/test/CodeGenOpenCL/preserve_vec3.cl
+++ clang/test/CodeGenOpenCL/preserve_vec3.cl
@@ -1,6 +1,7 @@
 // RUN: %clang_cc1 %s -emit-llvm -o - -triple spir-unknown-unknown 
-fpreserve-vec3-type | FileCheck %s
 
 typedef char char3 __attribute__((ext_vector_type(3)));
+typedef char char8 __attribute__((ext_vector_type(8)));
 typedef short short3 __attribute__((ext_vector_type(3)));
 typedef double double2 __attribute__((ext_vector_type(2)));
 typedef float float3 __attribute__((ext_vector_type(3)));
@@ -38,6 +39,15 @@
   *b = __builtin_astype(*a, double2);
 }
 
+void kernel char8_to_short3(global short3 *a, global char8 *b) {
+  // CHECK-LABEL: spir_kernel void @char8_to_short3
+  // CHECK: %[[IN_BC:.*]] = bitcast <8 x i8> addrspace(1)* %b to <4 x i16> 
addrspace(1)*
+  // CHECK: %[[LOAD_B:.*]] = load <4 x i16>, <4 x i16> addrspace(1)* %[[IN_BC]]
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x i16> %[[LOAD_B]], <4 x i16> 
poison, <3 x i32> 
+  // CHECK: store <3 x i16> %[[ASTYPE]], <3 x i16> addrspace(1)* %a, align 8
+  *a = __builtin_astype(*b, short3);
+}
+
 void from_char3(char3 a, global int *out) {
   // CHECK-LABEL: void @from_char3
   // CHECK: %[[ASTYPE:.*]] = shufflevector <3 x i8> %a, <3 x i8> poison, <4 x 
i32> 
@@ -53,3 +63,19 @@
   // CHECK: store <4 x i16> %[[ASTYPE]], <4 x i16> addrspace(1)* %[[OUT_BC]]
   *out = __builtin_astype(a, long);
 }
+
+void scalar_to_char3(int a, global char3 *out) {
+  // CHECK-LABEL: void @scalar_to_char3
+  // CHECK: %[[IN_BC:.*]] = bitcast i32 %a to <4 x i8>
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x i8> %[[IN_BC]], <4 x i8> 
poison, <3 x i32> 
+  // CHECK: store <3 x i8> %[[ASTYPE]], <3 x i8> addrspace(1)* %out
+  *out = __builtin_astype(a, char3);
+}
+
+void scalar_to_short3(long a, global short3 *out) {
+  // CHECK-LABEL: void @scalar_to_short3
+  // CHECK: %[[IN_BC:.*]] = bitcast i64 %a to <4 x i16>
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x i16> %[[IN_BC]], <4 x i16> 
poison, <3 x i32> 
+  // CHECK: store <3 x i16> %[[ASTYPE]], <3 x i16> addrspace(1)* %out
+  *out = __builtin_astype(a, short3);
+}
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -4796,12 +4796,10 @@
   // to vec4 if the original type is not vec4, then a shuffle vector to
   // get a vec3.
   if (NumElementsSrc != 3 && NumElementsDst == 3) {
-if (!CGF.CGM.getCodeGenOpts().PreserveVec3Type) {
-  auto *Vec4Ty = llvm::FixedVectorType::get(
-  cast(DstTy)->getElementType(), 4);
-  Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(), Src,
- Vec4Ty);
-}
+auto *Vec4Ty = llvm::FixedVectorType::get(
+cast(DstTy)->getElementType(), 4);
+Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(), Src,
+   Vec4Ty);
 
 Src = ConvertVec3AndVec4(Builder, CGF, Src, 3);
 Src->setName("astype");


Index: clang/test/CodeGenOpenCL/preserve_vec3.cl
===
--- clang/test/CodeGenOpenCL/preserve_vec3.cl
+++ clang/test/CodeGenOpenCL/preserve_vec3.cl
@@ -1,6 +1,7 @@
 // RUN: %clang_cc1 %s -emit-llvm -o - -triple spir-unknown-unknown -fpreserve-vec3-type | FileCheck %s
 
 typedef char char3 __attribute__((ext_vector_type(3)));
+typedef char char8 __attribute__((ext_vector_type(8)));
 typedef short short3 __attribute__((ext_vector_type(3)));
 typedef double double2 __attribute__((ext_vector_type(2)));
 typedef float float3 __attribute__((ext_vector_type(3)));
@@ -38,6 +39,15 @@
   *b = __builtin_astype(*a, double2);
 }
 
+void kernel char8_to_short3(global short3 *a, global char8 *b) {
+  // CHECK-LABEL: spir_kernel void @char8_to_short3
+  // CHECK: %[[IN_BC:.*]] = bitcast <8 x i8> addrspace(1)* %b to <4 x i16> addrspace(1)*
+  // CHECK: %[[LOAD_B:.*]] = load <4 x i16>, <4 x i16> addrspace(1)* %[[IN_BC]]
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x i16> %[[LOAD_B]], <4 x i16> poison, <3 x i32> 
+  // CHECK: store <3 x i16> %[[ASTYPE]], <3 x i16> addrspace(1)* %a, align 8
+  *a = __builtin_astype(*b, short3);
+}
+
 void from_char3(char3 a, global int *out) {
   // CHECK-LABEL: void @from_char3
   // CHECK: %[[ASTYPE:.*]] = shufflevector <3 x i8> %a, <3 x i8> poison, <4 x i32> 
@@ -53,3 +63,19 @@
   // CHECK: store <4 x i16> %[[ASTYPE]], <4 x i16> addrspace(1)* %[[OUT_BC]]
   *out = __builtin_astype(a, long);
 }
+
+void scalar_to_char3(int a, global c

[PATCH] D108470: [OpenCL] Fix as_type3 invalid store creation

[JinGu Kang via Phabricator via cfe-commits]

jaykang10 added a comment.

If possible, can you add more tests for different types as previous patch 
please?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108470/new/

https://reviews.llvm.org/D108470

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108470: [OpenCL] Fix as_type3 invalid store creation

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh created this revision.
svenvh added reviewers: Anastasia, jaykang10.
svenvh added a project: clang.
Herald added subscribers: ldrumm, yaxunl.
svenvh requested review of this revision.
Herald added a subscriber: cfe-commits.

With -fpreserve-vec3-type enabled, a cast was not created when
converting from a non-vec3 type to a vec3 type, even though a
conversion to vec3 was performed.  This resulted in creation of
invalid store instructions.

Related to D107963 , but now for the to-vec3 
case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108470

Files:
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/CodeGenOpenCL/preserve_vec3.cl


Index: clang/test/CodeGenOpenCL/preserve_vec3.cl
===
--- clang/test/CodeGenOpenCL/preserve_vec3.cl
+++ clang/test/CodeGenOpenCL/preserve_vec3.cl
@@ -53,3 +53,11 @@
   // CHECK: store <4 x i16> %[[ASTYPE]], <4 x i16> addrspace(1)* %[[OUT_BC]]
   *out = __builtin_astype(a, long);
 }
+
+void to_char3(int a, global char3 *out) {
+  // CHECK-LABEL: void @to_char3
+  // CHECK: %[[IN_BC:.*]] = bitcast i32 %a to <4 x i8>
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x i8> %[[IN_BC]], <4 x i8> 
poison, <3 x i32> 
+  // CHECK: store <3 x i8> %[[ASTYPE]], <3 x i8> addrspace(1)* %out
+  *out = __builtin_astype(a, char3);
+}
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -4796,12 +4796,10 @@
   // to vec4 if the original type is not vec4, then a shuffle vector to
   // get a vec3.
   if (NumElementsSrc != 3 && NumElementsDst == 3) {
-if (!CGF.CGM.getCodeGenOpts().PreserveVec3Type) {
-  auto *Vec4Ty = llvm::FixedVectorType::get(
-  cast(DstTy)->getElementType(), 4);
-  Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(), Src,
- Vec4Ty);
-}
+auto *Vec4Ty = llvm::FixedVectorType::get(
+cast(DstTy)->getElementType(), 4);
+Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(), Src,
+   Vec4Ty);
 
 Src = ConvertVec3AndVec4(Builder, CGF, Src, 3);
 Src->setName("astype");


Index: clang/test/CodeGenOpenCL/preserve_vec3.cl
===
--- clang/test/CodeGenOpenCL/preserve_vec3.cl
+++ clang/test/CodeGenOpenCL/preserve_vec3.cl
@@ -53,3 +53,11 @@
   // CHECK: store <4 x i16> %[[ASTYPE]], <4 x i16> addrspace(1)* %[[OUT_BC]]
   *out = __builtin_astype(a, long);
 }
+
+void to_char3(int a, global char3 *out) {
+  // CHECK-LABEL: void @to_char3
+  // CHECK: %[[IN_BC:.*]] = bitcast i32 %a to <4 x i8>
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x i8> %[[IN_BC]], <4 x i8> poison, <3 x i32> 
+  // CHECK: store <3 x i8> %[[ASTYPE]], <3 x i8> addrspace(1)* %out
+  *out = __builtin_astype(a, char3);
+}
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -4796,12 +4796,10 @@
   // to vec4 if the original type is not vec4, then a shuffle vector to
   // get a vec3.
   if (NumElementsSrc != 3 && NumElementsDst == 3) {
-if (!CGF.CGM.getCodeGenOpts().PreserveVec3Type) {
-  auto *Vec4Ty = llvm::FixedVectorType::get(
-  cast(DstTy)->getElementType(), 4);
-  Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(), Src,
- Vec4Ty);
-}
+auto *Vec4Ty = llvm::FixedVectorType::get(
+cast(DstTy)->getElementType(), 4);
+Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(), Src,
+   Vec4Ty);
 
 Src = ConvertVec3AndVec4(Builder, CGF, Src, 3);
 Src->setName("astype");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits