clang/OpenCL: Add baseline test showing broken codegen

[arsenm]

No description provided.

[arsenm]

• clang: Remove dest LangAS argument from performAddrSpaceCast #138866
• clang: Read the address space from the ABIArgInfo #138865
• clang/OpenCL: Fix special casing OpenCL in call emission #138864
• clang: Fix broken implicit cast to generic address space #138863
• clang/OpenCL: Add baseline test showing broken codegen #138862 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-backend-amdgpu

Author: Matt Arsenault (arsenm)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/138862.diff

2 Files Affected:

• (modified) clang/test/CodeGenCXX/amdgcn-automatic-variable.cpp (+24)
• (modified) clang/test/CodeGenOpenCL/amdgcn-automatic-variable.cl (+45)

diff --git a/clang/test/CodeGenCXX/amdgcn-automatic-variable.cpp b/clang/test/CodeGenCXX/amdgcn-automatic-variable.cpp
index c1f9310141000..0f425d78c3332 100644
--- a/clang/test/CodeGenCXX/amdgcn-automatic-variable.cpp
+++ b/clang/test/CodeGenCXX/amdgcn-automatic-variable.cpp
@@ -133,3 +133,27 @@ void func7() {
   use(&x);
 }
 
+#define __private __attribute__((opencl_private))
+
+// CHECK-LABEL: @_Z34explicit_private_address_space_ptrv(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[PLONG:%.*]] = alloca i64, align 8, addrspace(5)
+// CHECK-NEXT:    [[PLONGP:%.*]] = alloca ptr, align 8, addrspace(5)
+// CHECK-NEXT:    [[PLONGP_B:%.*]] = alloca ptr addrspace(5), align 4, addrspace(5)
+// CHECK-NEXT:    [[PLONG_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[PLONG]] to ptr
+// CHECK-NEXT:    [[PLONGP_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[PLONGP]] to ptr
+// CHECK-NEXT:    [[PLONGP_B_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[PLONGP_B]] to ptr
+// CHECK-NEXT:    store ptr [[PLONG_ASCAST]], ptr [[PLONGP_ASCAST]], align 8
+// CHECK-NEXT:    [[PLONG_ASCAST_ASCAST:%.*]] = addrspacecast ptr [[PLONG_ASCAST]] to ptr addrspace(5)
+// CHECK-NEXT:    store ptr addrspace(5) [[PLONG_ASCAST_ASCAST]], ptr [[PLONGP_B_ASCAST]], align 4
+// CHECK-NEXT:    [[TMP0:%.*]] = load ptr, ptr [[PLONGP_ASCAST]], align 8
+// CHECK-NEXT:    store i64 8, ptr [[TMP0]], align 8
+// CHECK-NEXT:    ret void
+//
+void explicit_private_address_space_ptr() {
+  long plong;
+  long *plongp = &plong;
+
+  __private long *plongp_b = (__private long *)&plong;
+  *plongp = 8;
+}
diff --git a/clang/test/CodeGenOpenCL/amdgcn-automatic-variable.cl b/clang/test/CodeGenOpenCL/amdgcn-automatic-variable.cl
index dba6519966eb5..b252f1041d68c 100644
--- a/clang/test/CodeGenOpenCL/amdgcn-automatic-variable.cl
+++ b/clang/test/CodeGenOpenCL/amdgcn-automatic-variable.cl
@@ -109,3 +109,48 @@ void func2(void) {
 void func3(void) {
   float a[16][1] = {{0.}};
 }
+
+// CL12-LABEL: define dso_local void @wrong_store_type_private_pointer_alloca(
+// CL12-SAME: ) #[[ATTR0]] {
+// CL12-NEXT:  [[ENTRY:.*:]]
+// CL12-NEXT:    [[PLONG:%.*]] = alloca i64, align 8, addrspace(5)
+// CL12-NEXT:    [[PLONGP:%.*]] = alloca ptr addrspace(5), align 4, addrspace(5)
+// CL12-NEXT:    [[GLONGP:%.*]] = alloca ptr addrspace(5), align 4, addrspace(5)
+// CL12-NEXT:    store i64 5, ptr addrspace(5) [[PLONG]], align 8
+// CL12-NEXT:    store ptr addrspace(5) [[PLONG]], ptr addrspace(5) [[PLONGP]], align 4
+// CL12-NEXT:    [[TMP0:%.*]] = load ptr addrspace(5), ptr addrspace(5) [[PLONGP]], align 4
+// CL12-NEXT:    store i64 8, ptr addrspace(5) [[TMP0]], align 8
+// CL12-NEXT:    store ptr addrspace(5) [[PLONG]], ptr addrspace(5) [[GLONGP]], align 4
+// CL12-NEXT:    [[TMP1:%.*]] = load ptr addrspace(5), ptr addrspace(5) [[GLONGP]], align 4
+// CL12-NEXT:    store i64 9, ptr addrspace(5) [[TMP1]], align 8
+// CL12-NEXT:    ret void
+//
+// CL20-LABEL: define dso_local void @wrong_store_type_private_pointer_alloca(
+// CL20-SAME: ) #[[ATTR0]] {
+// CL20-NEXT:  [[ENTRY:.*:]]
+// CL20-NEXT:    [[PLONG:%.*]] = alloca i64, align 8, addrspace(5)
+// CL20-NEXT:    [[PLONGP:%.*]] = alloca ptr addrspace(5), align 4, addrspace(5)
+// CL20-NEXT:    [[GLONGP:%.*]] = alloca ptr, align 8, addrspace(5)
+// CL20-NEXT:    [[PLONG_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[PLONG]] to ptr
+// CL20-NEXT:    [[PLONGP_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[PLONGP]] to ptr
+// CL20-NEXT:    [[GLONGP_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[GLONGP]] to ptr
+// CL20-NEXT:    store i64 5, ptr [[PLONG_ASCAST]], align 8
+// CL20-NEXT:    store ptr [[PLONG_ASCAST]], ptr [[PLONGP_ASCAST]], align 4
+// CL20-NEXT:    [[TMP0:%.*]] = load ptr addrspace(5), ptr [[PLONGP_ASCAST]], align 4
+// CL20-NEXT:    store i64 8, ptr addrspace(5) [[TMP0]], align 8
+// CL20-NEXT:    store ptr [[PLONG_ASCAST]], ptr [[GLONGP_ASCAST]], align 8
+// CL20-NEXT:    [[TMP1:%.*]] = load ptr, ptr [[GLONGP_ASCAST]], align 8
+// CL20-NEXT:    store i64 9, ptr [[TMP1]], align 8
+// CL20-NEXT:    ret void
+//
+void wrong_store_type_private_pointer_alloca() {
+  long plong = 5;
+
+  // This needs to write an addrspace(5) pointer to the temporary alloca
+  __private long *plongp = &plong;
+  *plongp = 8;
+
+  // This needs to write an addrspace(0) pointer to the temporary alloca in CL2.0
+  long *glongp = &plong;
+  *glongp = 9;
+}

[Artem-B]

It would be useful to mention what we're currently doing wrong, as a FIXME/TODO.
One could dig through the IR checks, but it's much easier when one knows what to look for, specifically.

[Artem-B]

I'd split it into two separate tests -- one for generic pointer, and one for private. That is, unless the issue is when both pointers are needed to reproduce the issue. Right now the checks have way too many [PG]LONGP? that require an effort to keep track of.

It may be the case where semantically sensible naming in the source code that uses prefix/suffix makes things harder to read when it comes to the test checks. It's a nit. If the test in the current shape works for you I'm OK with it. I just found it to be not quite "painfully obvious".

How about something like this:

  long data = 5;

  // This needs to write an addrspace(5) pointer to the temporary alloca
  __private long *pvtp = &data;
  *pvtp = 8;
  long *genp = &data;
  *genp = 9;

Maybe, also make genp and pvtp static, so their own allocas do not clutter the test checks. In the checks themselves, I'd also rename generic _ASCAST so it reflects the AS we cast to. E.g. _GEN or _PVT, so the reader does not have to back-track to where the value is captured.

[arsenm]

Merge activity

• May 8, 1:36 AM EDT: A user started a stack merge that includes this pull request via Graphite.
• May 8, 1:38 AM EDT: Graphite rebased this pull request as part of a merge.
• May 8, 1:41 AM EDT: Graphite rebased this pull request as part of a merge.
• May 8, 1:43 AM EDT: Graphite rebased this pull request as part of a merge.
• May 8, 1:46 AM EDT: @arsenm merged this pull request with Graphite.