- 
                Notifications
    
You must be signed in to change notification settings  - Fork 15.1k
 
[clang][SPIR][SPIRV] Don't generate constant NULL from addrspacecast generic NULL #165353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang][SPIR][SPIRV] Don't generate constant NULL from addrspacecast generic NULL #165353
Conversation
…generic NULL Fix a regression caused by 1ffff05. OpenCL spec forbids casting between generic and constant address space pointers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a regression in SPIR/SPIRV code generation where casting between generic and constant address space pointers was incorrectly allowed, violating the OpenCL specification. The fix ensures that constant address space NULL pointers are generated directly instead of through address space casting from generic NULL.
- Adds explicit handling for 
opencl_constantaddress space in thegetNullPointerfunction - Adds test coverage to verify correct LLVM IR generation for constant address space NULL pointers
 
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description | 
|---|---|
| clang/lib/CodeGen/Targets/SPIR.cpp | Modified getNullPointer to include opencl_constant address space in the condition that returns direct constant NULL pointers | 
| clang/test/CodeGenOpenCL/nullptr.cl | Added new test file to verify correct handling of NULL pointers in constant address space | 
| 
          
 @llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-clang Author: Wenju He (wenju-he) ChangesFix a regression caused by 1ffff05. Full diff: https://github.com/llvm/llvm-project/pull/165353.diff 2 Files Affected: 
 diff --git a/clang/lib/CodeGen/Targets/SPIR.cpp b/clang/lib/CodeGen/Targets/SPIR.cpp
index 15d0b353d748c..abd049aca0ed7 100644
--- a/clang/lib/CodeGen/Targets/SPIR.cpp
+++ b/clang/lib/CodeGen/Targets/SPIR.cpp
@@ -260,7 +260,8 @@ CommonSPIRTargetCodeGenInfo::getNullPointer(const CodeGen::CodeGenModule &CGM,
   LangAS AS = QT->getUnqualifiedDesugaredType()->isNullPtrType()
                   ? LangAS::Default
                   : QT->getPointeeType().getAddressSpace();
-  if (AS == LangAS::Default || AS == LangAS::opencl_generic)
+  if (AS == LangAS::Default || AS == LangAS::opencl_generic ||
+      AS == LangAS::opencl_constant)
     return llvm::ConstantPointerNull::get(PT);
 
   auto &Ctx = CGM.getContext();
diff --git a/clang/test/CodeGenOpenCL/nullptr.cl b/clang/test/CodeGenOpenCL/nullptr.cl
new file mode 100644
index 0000000000000..6d203fce91dde
--- /dev/null
+++ b/clang/test/CodeGenOpenCL/nullptr.cl
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -no-enable-noundef-analysis %s -cl-std=CL2.0 -include opencl-c.h -triple spir64 -emit-llvm -o - | FileCheck %s
+
+// CHECK: @constant_p_NULL =
+// CHECK-SAME: addrspace(1) global ptr addrspace(2) null, align 8
+constant char *constant_p_NULL = NULL;
+
+// CHECK-LABEL: cmp_constant
+// CHECK: icmp eq ptr addrspace(2) %p, null
+char cmp_constant(constant char* p) {
+  if (p != 0)
+    return *p;
+  else
+    return 0;
+}
 | 
    
| 
          
 @llvm/pr-subscribers-clang-codegen Author: Wenju He (wenju-he) ChangesFix a regression caused by 1ffff05. Full diff: https://github.com/llvm/llvm-project/pull/165353.diff 2 Files Affected: 
 diff --git a/clang/lib/CodeGen/Targets/SPIR.cpp b/clang/lib/CodeGen/Targets/SPIR.cpp
index 15d0b353d748c..abd049aca0ed7 100644
--- a/clang/lib/CodeGen/Targets/SPIR.cpp
+++ b/clang/lib/CodeGen/Targets/SPIR.cpp
@@ -260,7 +260,8 @@ CommonSPIRTargetCodeGenInfo::getNullPointer(const CodeGen::CodeGenModule &CGM,
   LangAS AS = QT->getUnqualifiedDesugaredType()->isNullPtrType()
                   ? LangAS::Default
                   : QT->getPointeeType().getAddressSpace();
-  if (AS == LangAS::Default || AS == LangAS::opencl_generic)
+  if (AS == LangAS::Default || AS == LangAS::opencl_generic ||
+      AS == LangAS::opencl_constant)
     return llvm::ConstantPointerNull::get(PT);
 
   auto &Ctx = CGM.getContext();
diff --git a/clang/test/CodeGenOpenCL/nullptr.cl b/clang/test/CodeGenOpenCL/nullptr.cl
new file mode 100644
index 0000000000000..6d203fce91dde
--- /dev/null
+++ b/clang/test/CodeGenOpenCL/nullptr.cl
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -no-enable-noundef-analysis %s -cl-std=CL2.0 -include opencl-c.h -triple spir64 -emit-llvm -o - | FileCheck %s
+
+// CHECK: @constant_p_NULL =
+// CHECK-SAME: addrspace(1) global ptr addrspace(2) null, align 8
+constant char *constant_p_NULL = NULL;
+
+// CHECK-LABEL: cmp_constant
+// CHECK: icmp eq ptr addrspace(2) %p, null
+char cmp_constant(constant char* p) {
+  if (p != 0)
+    return *p;
+  else
+    return 0;
+}
 | 
    
Co-authored-by: Alexey Bader <alexey.bader@intel.com>
          
 Doesn't really matter for the purposes of what the IR, it's a question of what the SPIRV machine allows  | 
    
        
          
                clang/test/CodeGenOpenCL/nullptr.cl
              
                Outdated
          
        
      | @@ -0,0 +1,13 @@ | |||
| // RUN: %clang_cc1 -no-enable-noundef-analysis %s -cl-std=CL2.0 -include opencl-c.h -triple spir64 -emit-llvm -o - | FileCheck %s | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test name is very broad for what this is testing. Can you merge this with an existing null test, or make this more comprehensive for all the address spaces (I thought we had one of those somewhere already, at least with amdgp run lines)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test name is very broad for what this is testing. Can you merge this with an existing null test, or make this more comprehensive for all the address spaces (I thought we had one of those somewhere already, at least with amdgp run lines)
done. The test was extracted from amdgpu-nullptr.cl
I have renamed amdgpu-nullptr.cl to nullptr.cl and added two spir64 run lines to the test.
The test now probably exposes two issues for spir64 targets:
- folding is incorrect for spir64 target
 
// SPIR64: @fold_int2 ={{.*}} local_unnamed_addr addrspace(1) global i32 13, align 4
// AMDGCN: @fold_int2 ={{.*}} local_unnamed_addr addrspace(1) global i32 12, align 4
int fold_int2 = (int) ((private void*)0 + 13);
- struct initialization is incorrect for spir64 target
 
typedef struct {
  private char *p1;
  local char *p2;
  constant char *p3;
  global char *p4;
  generic char *p5;
} StructTy1;
// SPIR64-NOOPT: @test_static_var_local.SS1 = internal addrspace(1) global %struct.StructTy1 zeroinitializer, align 8
// AMDGCN-NOOPT: @test_static_var_local.SS1 = internal addrspace(1) global %struct.StructTy1 { ptr addrspace(5) addrspacecast (ptr null to ptr addrspace(5)), ptr addrspace(3) addrspacecast (ptr null to ptr addrspace(3)), ptr addrspace(4) null, ptr addrspace(1) null, ptr null }, align 8
void test_static_var_local(void) {
          
 reworded to   | 
    
…generic NULL (llvm#165353) Fix a regression caused by 1ffff05. OpenCL/SPIRV generic address space doesn't cover constant address space. --------- Co-authored-by: Alexey Bader <alexey.bader@intel.com>
Fix a regression caused by 1ffff05.
OpenCL/SPIRV generic address space doesn't cover constant address space.