-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang][SPIR][SPIRV] Materialize non-generic null pointers via addrspacecast #161773
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
base: main
Are you sure you want to change the base?
[clang][SPIR][SPIRV] Materialize non-generic null pointers via addrspacecast #161773
Conversation
…acecast LLVM models ConstantPointerNull as all-zero, but some GPUs (e.g. AMDGPU and our downstream GPU target) use a non-zero sentinel for null in private / local address spaces. SPIR-V is a supported input for our GPU target. This PR preserves a canonical zero form in the generic AS while allowing later lowering to substitute the target’s real sentinel.
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Wenju He (wenju-he) ChangesLLVM models ConstantPointerNull as all-zero, but some GPUs (e.g. AMDGPU and our downstream GPU target) use a non-zero sentinel for null in private / local address spaces. Full diff: https://github.com/llvm/llvm-project/pull/161773.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/Targets/SPIR.cpp b/clang/lib/CodeGen/Targets/SPIR.cpp
index 4aa63143a66cd..b1c1d65b4f5e1 100644
--- a/clang/lib/CodeGen/Targets/SPIR.cpp
+++ b/clang/lib/CodeGen/Targets/SPIR.cpp
@@ -61,6 +61,9 @@ class CommonSPIRTargetCodeGenInfo : public TargetCodeGenInfo {
QualType SampledType, CodeGenModule &CGM) const;
void
setOCLKernelStubCallingConvention(const FunctionType *&FT) const override;
+ llvm::Constant *getNullPointer(const CodeGen::CodeGenModule &CGM,
+ llvm::PointerType *T,
+ QualType QT) const override;
};
class SPIRVTargetCodeGenInfo : public CommonSPIRTargetCodeGenInfo {
public:
@@ -240,6 +243,27 @@ void CommonSPIRTargetCodeGenInfo::setOCLKernelStubCallingConvention(
FT, FT->getExtInfo().withCallingConv(CC_SpirFunction));
}
+// LLVM currently assumes a null pointer has the bit pattern 0, but some GPU
+// targets use a non-zero encoding for null in certain address spaces.
+// Because SPIR(-V) is a virtual target and the bit pattern of a non-generic
+// null is unspecified, materialize non-generic null via an addrspacecast from
+// the generic null.
+// This allows later lowering to substitute the target’s real sentinel value.
+llvm::Constant *
+CommonSPIRTargetCodeGenInfo::getNullPointer(const CodeGen::CodeGenModule &CGM,
+ llvm::PointerType *PT,
+ QualType QT) const {
+ auto &Ctx = CGM.getContext();
+ if (PT->getAddressSpace() ==
+ Ctx.getTargetAddressSpace(LangAS::opencl_generic))
+ return llvm::ConstantPointerNull::get(PT);
+
+ auto NPT = llvm::PointerType::get(
+ PT->getContext(), Ctx.getTargetAddressSpace(LangAS::opencl_generic));
+ return llvm::ConstantExpr::getAddrSpaceCast(
+ llvm::ConstantPointerNull::get(NPT), PT);
+}
+
LangAS
SPIRVTargetCodeGenInfo::getGlobalVarAddressSpace(CodeGenModule &CGM,
const VarDecl *D) const {
diff --git a/clang/test/CodeGenOpenCL/builtins.cl b/clang/test/CodeGenOpenCL/builtins.cl
index aa666c7b671b9..708d1b84f4838 100644
--- a/clang/test/CodeGenOpenCL/builtins.cl
+++ b/clang/test/CodeGenOpenCL/builtins.cl
@@ -62,19 +62,19 @@ void testBranchingOnAddressSpaceCast(generic long* ptr) {
if (to_global(ptr))
(void)0;
// CHECK: [[P:%[0-9]+]] = call spir_func [[GLOBAL_VOID:ptr addrspace\(1\)]] @__to_global([[GENERIC_VOID:ptr addrspace\(4\)]] {{%[0-9]+}})
- // CHECK-NEXT: [[BOOL:%[a-z0-9]+]] = icmp ne ptr addrspace(1) [[P]], null
+ // CHECK-NEXT: [[BOOL:%[a-z0-9]+]] = icmp ne ptr addrspace(1) [[P]], addrspacecast (ptr addrspace(4) null to ptr addrspace(1))
// CHECK-NEXT: br i1 [[BOOL]]
if (to_local(ptr))
(void)0;
// CHECK: [[P:%[0-9]+]] = call spir_func [[LOCAL_VOID:ptr addrspace\(3\)]] @__to_local([[GENERIC_VOID]] {{%[0-9]+}})
- // CHECK-NEXT: [[BOOL:%[a-z0-9]+]] = icmp ne ptr addrspace(3) [[P]], null
+ // CHECK-NEXT: [[BOOL:%[a-z0-9]+]] = icmp ne ptr addrspace(3) [[P]], addrspacecast (ptr addrspace(4) null to ptr addrspace(3))
// CHECK-NEXT: br i1 [[BOOL]]
if (to_private(ptr))
(void)0;
// CHECK: [[P:%[0-9]+]] = call spir_func [[PRIVATE_VOID:ptr]] @__to_private([[GENERIC_VOID]] {{%[0-9]+}})
- // CHECK-NEXT: [[BOOL:%[a-z0-9]+]] = icmp ne ptr [[P]], null
+ // CHECK-NEXT: [[BOOL:%[a-z0-9]+]] = icmp ne ptr [[P]], addrspacecast (ptr addrspace(4) null to ptr)
// CHECK-NEXT: br i1 [[BOOL]]
}
diff --git a/clang/test/CodeGenSYCL/address-space-conversions.cpp b/clang/test/CodeGenSYCL/address-space-conversions.cpp
index fa7acb0d99433..ea52be53ea906 100644
--- a/clang/test/CodeGenSYCL/address-space-conversions.cpp
+++ b/clang/test/CodeGenSYCL/address-space-conversions.cpp
@@ -35,9 +35,9 @@ void tmpl(T t) {}
// CHECK-DAG: [[LOC]].ascast = addrspacecast ptr [[LOC]] to ptr addrspace(4)
// CHECK-DAG: [[PRIV]].ascast = addrspacecast ptr [[PRIV]] to ptr addrspace(4)
LOC = nullptr;
- // CHECK-DAG: store ptr addrspace(3) null, ptr addrspace(4) [[LOC]].ascast, align 8
+ // CHECK-DAG: store ptr addrspace(3) addrspacecast (ptr addrspace(4) null to ptr addrspace(3)), ptr addrspace(4) [[LOC]].ascast, align 8
GLOB = nullptr;
- // CHECK-DAG: store ptr addrspace(1) null, ptr addrspace(4) [[GLOB]].ascast, align 8
+ // CHECK-DAG: store ptr addrspace(1) addrspacecast (ptr addrspace(4) null to ptr addrspace(1)), ptr addrspace(4) [[GLOB]].ascast, align 8
// Explicit conversions
// From named address spaces to default address space
|
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 modifies the SPIR and SPIRV code generation to properly handle null pointers in non-generic address spaces. Instead of using direct null values for private/local address spaces, it materializes these nulls via addrspacecast from generic null pointers to accommodate GPU targets that use non-zero sentinel values for null in certain address spaces.
- Implements a new
getNullPointer
method in SPIR target code generation that creates addrspacecast expressions for non-generic address spaces - Updates test expectations to verify the new addrspacecast pattern instead of direct null comparisons
- Preserves canonical zero form in generic address space while allowing target-specific null representations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
clang/lib/CodeGen/Targets/SPIR.cpp | Adds getNullPointer override that materializes non-generic nulls via addrspacecast from generic null |
clang/test/CodeGenSYCL/address-space-conversions.cpp | Updates test expectations to verify addrspacecast pattern for null pointer stores |
clang/test/CodeGenOpenCL/builtins.cl | Updates test expectations to verify addrspacecast pattern for null pointer comparisons |
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
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
// the generic null. | ||
// This allows later lowering to substitute the target’s real sentinel value. | ||
llvm::Constant * | ||
CommonSPIRTargetCodeGenInfo::getNullPointer(const CodeGen::CodeGenModule &CGM, |
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.
Can you directly copy the AMDGPU implementation, which checks against getTargetNullPointerValue
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.
Can you directly copy the AMDGPU implementation, which checks against getTargetNullPointerValue
getTargetNullPointerValue
requires defining getNullPointerValue in BaseSPIRTargetInfo, like
llvm-project/clang/lib/Basic/Targets/AMDGPU.h
Lines 424 to 430 in 441f0c7
uint64_t getNullPointerValue(LangAS AS) const override { | |
// FIXME: Also should handle region. | |
return (AS == LangAS::opencl_local || AS == LangAS::opencl_private || | |
AS == LangAS::sycl_local || AS == LangAS::sycl_private) | |
? ~0 | |
: 0; | |
} |
However, we don't know the return value of getNullPointerValue in BaseSPIRTargetInfo except for generic address space, right?
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.
Hm, seems like getNullPointerValue should be able to fail. My main worry here is special casing generic address space, as opposed to default address space
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.
Hm, seems like getNullPointerValue should be able to fail. My main worry here is special casing generic address space, as opposed to default address space
thanks @arsenm, you're right that the default AS is not checked.
I copied the logic from ASTContext::getTargetNullPointerValue into CommonSPIRTargetCodeGenInfo::getNullPointer in 5c5e13e to avoid changing return type of both getNullPointerValue and getTargetNullPointerValue to std::optional<uint64_t>. Since getTargetNullPointerValue is used in many places, changing its return type looks like unnecessary churn.
…deGenInfo::getNullPointer
LLVM models ConstantPointerNull as all-zero, but some GPUs (e.g. AMDGPU and our downstream GPU target) use a non-zero sentinel for null in private / local address spaces.
SPIR-V is a supported input for our GPU target. This PR preserves a canonical zero form in the generic AS while allowing later lowering to substitute the target’s real sentinel.