[PATCH] D26196: AMDGPU: Translate null pointers in private and local addr space
rjmccall added a comment. In https://reviews.llvm.org/D26196#587135, @yaxunl wrote: > In https://reviews.llvm.org/D26196#586433, @rjmccall wrote: > > > I think there are a number of conceptual questions here, chiefly among them > > whether an llvm::ConstantPointerNull in an address-space type should have > > the correct representation for the target. If the decision for that is > > "no", then ok, we can produce an explicit representation in IRGen; however, > > we will need to update quite a bit of code in order to do so correctly. > > > As Tom and Matt pointed out, llvm::ConstantPointerNull is the desired > representation for a null pointer. However, currently LLVM assumes that it > has 0 value extensively, and there is no plan to change that in a foreseeable > future. This patch is intended to provide a workaround for this restriction > by representing non-zero null pointer in an alternative way. Yes, I understand the goal of this patch, and I am fine with working around the LLVM limitation in the frontend. I will, however, point out that Clang's assumptions about the representation of null are not necessarily any less extensive than LLVM's. >> In general, frontend address spaces don't necessarily correspond to IR >> address spaces. All of your address-space operations need to be >> parameterized with a frontend address space (or both a source and dest >> address space for conversions). The question you should be keeping in mind >> when designing these APIs is "what if there were an address space defined to >> be exactly the generic address space, only with a different null value?" > > Since non-zero null pointer is usually resulted from target restrictions (as > in the case of amdgpu target), whether a null pointer needs a special > representation only depends on the target or IR address space. Therefore we > do not need to consider the address space in the source language. Hopefully > this can simplify the implementation. Please do as I laid out and base this decision on the source-language address space. It will not significantly complicate the implementation. https://reviews.llvm.org/D26196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26196: AMDGPU: Translate null pointers in private and local addr space
arsenm added a comment. In https://reviews.llvm.org/D26196#586433, @rjmccall wrote: > I think there are a number of conceptual questions here, chiefly among them > whether an llvm::ConstantPointerNull in an address-space type should have the > correct representation for the target. If the decision for that is "no", > then ok, we can produce an explicit representation in IRGen; however, we will > need to update quite a bit of code in order to do so correctly. I think the answer for this is yes, but doing so would be a pretty involved project. In https://reviews.llvm.org/D26196#586723, @Anastasia wrote: > My understanding of NULL constant in IR was that it doesn't assume any > specific integer value (i.e. 0). And currently as I can see it is lowered > very late in LLVM backend during the code selection phase. > > Looking at the lowering code in LLVM, it seems like it shouldn't be too > difficult to special case the NULL value based on the address space: > http://llvm.org/docs/doxygen/html/SelectionDAGBuilder_8cpp_source.html#l01039 > instead of using hard-coded 0 at it's done now. > > My opinion is that we should try to lower the code as late as possible and > Clang doesn't seem like an ideal place for this. Also this change adds extra > overhead by adding extra addresspacecast instructions and complicates > frontend implementation. I don't see immediate benefit in changing this in > the frontend compared to changing LLVM code selection phase instead. This is essentially moving the lowering of the constant to as late as possible by moving it to the addrspacecast lowering. I think it would be a pretty substantial project to add non-0 null pointer support, but would be better long term. It is treated as isNull, so computeKnownBits and other places already treat null as an alias for 0. Tracking down all of these would be a time consuming endeavor https://reviews.llvm.org/D26196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26196: AMDGPU: Translate null pointers in private and local addr space
tstellarAMD added a comment. In https://reviews.llvm.org/D26196#586723, @Anastasia wrote: > My understanding of NULL constant in IR was that it doesn't assume any > specific integer value (i.e. 0). And currently as I can see it is lowered > very late in LLVM backend during the code selection phase. It is assumed to be zero in some places. For example: https://github.com/llvm-mirror/llvm/blob/master/lib/IR/ConstantFold.cpp#L620 https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/ValueTracking.cpp#L1512 -Tom https://reviews.llvm.org/D26196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26196: AMDGPU: Translate null pointers in private and local addr space
Anastasia added a comment. My understanding of NULL constant in IR was that it doesn't assume any specific integer value (i.e. 0). And currently as I can see it is lowered very late in LLVM backend during the code selection phase. Looking at the lowering code in LLVM, it seems like it shouldn't be too difficult to special case the NULL value based on the address space: http://llvm.org/docs/doxygen/html/SelectionDAGBuilder_8cpp_source.html#l01039 instead of using hard-coded 0 at it's done now. My opinion is that we should try to lower the code as late as possible and Clang doesn't seem like an ideal place for this. Also this change adds extra overhead by adding extra addresspacecast instructions and complicates frontend implementation. I don't see immediate benefit in changing this in the frontend compared to changing LLVM code selection phase instead. https://reviews.llvm.org/D26196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26196: AMDGPU: Translate null pointers in private and local addr space
rjmccall added a comment. I think there are a number of conceptual questions here, chiefly among them whether an llvm::ConstantPointerNull in an address-space type should have the correct representation for the target. If the decision for that is "no", then ok, we can produce an explicit representation in IRGen; however, we will need to update quite a bit of code in order to do so correctly. In general, frontend address spaces don't necessarily correspond to IR address spaces. All of your address-space operations need to be parameterized with a frontend address space (or both a source and dest address space for conversions). The question you should be keeping in mind when designing these APIs is "what if there were an address space defined to be exactly the generic address space, only with a different null value?" IRGen has primitive knowledge about null pointer representations that are encoded in a bunch of places. Among them are: - Emitting zero-initialization. Fortunately, we already have this well-abstracted because of C++; search for isZeroInitializable. - Converting from a literal 0, nullptr_t etc. to a given pointer type; this is CK_NullToPointer. - Source-level conversions of a pointer type to boolean; this is CK_PointerToBoolean. - Inherent null-checks of pointer values; you'll need to search for CK_DerivedToBase and CK_BaseToDerived. You will also need to verify that the AST-level constant evaluator correctly distinguishes between CK_NullToPointer and CK_IntegerToPointer. I think you need to provide three basic operations on CodeGenTargetInfo: - Get a null pointer value in a given address space as an llvm::Constant. - Test whether a value in a given address space is a null value. - Convert a value from one address space to another. You'll need to make sure that everything in IRGen that does an address space conversion goes through this, and the default implementation should be the only thing that ever creates an LLVM addrspace cast. https://reviews.llvm.org/D26196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26196: AMDGPU: Translate null pointers in private and local addr space
yaxunl added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:7039 + if (CGM.getTarget().getTriple().getArch() != llvm::Triple::amdgcn || +(AS != Ctx.getTargetAddressSpace(LangAS::opencl_local) && AS != 0)) +return C; arsenm wrote: > Shouldn't the 0 be checking lang as::opencl_private? clang does not define enum for opencl_private. https://reviews.llvm.org/D26196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26196: AMDGPU: Translate null pointers in private and local addr space
yaxunl updated the summary for this revision.
yaxunl updated this revision to Diff 76792.
yaxunl added a comment.
Fixed translation of a pointer to bool. Added more tests.
https://reviews.llvm.org/D26196
Files:
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExprConstant.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenModule.h
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
test/CodeGenOpenCL/amdgpu-nullptr.cl
Index: test/CodeGenOpenCL/amdgpu-nullptr.cl
===
--- /dev/null
+++ test/CodeGenOpenCL/amdgpu-nullptr.cl
@@ -0,0 +1,325 @@
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -include opencl-c.h -triple amdgcn -emit-llvm -o - | FileCheck %s
+
+// Test 0 as initializer.
+
+// CHECK: @private_p = local_unnamed_addr addrspace(1) global i8* addrspacecast (i8 addrspace(4)* null to i8*), align 4
+private char *private_p = 0;
+
+// CHECK: @local_p = local_unnamed_addr addrspace(1) global i8 addrspace(3)* addrspacecast (i8 addrspace(4)* null to i8 addrspace(3)*), align 4
+local char *local_p = 0;
+
+// CHECK: @global_p = local_unnamed_addr addrspace(1) global i8 addrspace(1)* null, align 4
+global char *global_p = 0;
+
+// CHECK: @constant_p = local_unnamed_addr addrspace(1) global i8 addrspace(2)* null, align 4
+constant char *constant_p = 0;
+
+// CHECK: @generic_p = local_unnamed_addr addrspace(1) global i8 addrspace(4)* null, align 4
+generic char *generic_p = 0;
+
+// Test NULL as initializer.
+
+// CHECK: @private_p_NULL = local_unnamed_addr addrspace(1) global i8* addrspacecast (i8 addrspace(4)* null to i8*), align 4
+private char *private_p_NULL = NULL;
+
+// CHECK: @local_p_NULL = local_unnamed_addr addrspace(1) global i8 addrspace(3)* addrspacecast (i8 addrspace(4)* null to i8 addrspace(3)*), align 4
+local char *local_p_NULL = NULL;
+
+// CHECK: @global_p_NULL = local_unnamed_addr addrspace(1) global i8 addrspace(1)* null, align 4
+global char *global_p_NULL = NULL;
+
+// CHECK: @constant_p_NULL = local_unnamed_addr addrspace(1) global i8 addrspace(2)* null, align 4
+constant char *constant_p_NULL = NULL;
+
+// CHECK: @generic_p_NULL = local_unnamed_addr addrspace(1) global i8 addrspace(4)* null, align 4
+generic char *generic_p_NULL = NULL;
+
+// Test comparison with 0.
+
+// CHECK-LABEL: cmp_private
+// CHECK: icmp eq i8* %p, addrspacecast (i8 addrspace(4)* null to i8*)
+void cmp_private(private char* p) {
+ if (p != 0)
+*p = 0;
+}
+
+// CHECK-LABEL: cmp_local
+// CHECK: icmp eq i8 addrspace(3)* %p, addrspacecast (i8 addrspace(4)* null to i8 addrspace(3)*)
+void cmp_local(local char* p) {
+ if (p != 0)
+*p = 0;
+}
+
+// CHECK-LABEL: cmp_global
+// CHECK: icmp eq i8 addrspace(1)* %p, null
+void cmp_global(global char* p) {
+ if (p != 0)
+*p = 0;
+}
+
+// CHECK-LABEL: cmp_constant
+// CHECK: icmp eq i8 addrspace(2)* %p, null
+char cmp_constant(constant char* p) {
+ if (p != 0)
+return *p;
+ else
+return 0;
+}
+
+// CHECK-LABEL: cmp_generic
+// CHECK: icmp eq i8 addrspace(4)* %p, null
+void cmp_generic(generic char* p) {
+ if (p != 0)
+*p = 0;
+}
+
+// Test comparison with NULL.
+
+// CHECK-LABEL: cmp_NULL_private
+// CHECK: icmp eq i8* %p, addrspacecast (i8 addrspace(4)* null to i8*)
+void cmp_NULL_private(private char* p) {
+ if (p != NULL)
+*p = 0;
+}
+
+// CHECK-LABEL: cmp_NULL_local
+// CHECK: icmp eq i8 addrspace(3)* %p, addrspacecast (i8 addrspace(4)* null to i8 addrspace(3)*)
+void cmp_NULL_local(local char* p) {
+ if (p != NULL)
+*p = 0;
+}
+
+// CHECK-LABEL: cmp_NULL_global
+// CHECK: icmp eq i8 addrspace(1)* %p, addrspacecast (i8 addrspace(4)* null to i8 addrspace(1)*)
+void cmp_NULL_global(global char* p) {
+ if (p != NULL)
+*p = 0;
+}
+
+// CHECK-LABEL: cmp_NULL_constant
+// CHECK: icmp eq i8 addrspace(2)* %p, addrspacecast (i8 addrspace(4)* null to i8 addrspace(2)*)
+char cmp_NULL_constant(constant char* p) {
+ if (p != NULL)
+return *p;
+ else
+return 0;
+}
+
+// CHECK-LABEL: cmp_NULL_generic
+// CHECK: icmp eq i8 addrspace(4)* %p, null
+void cmp_NULL_generic(generic char* p) {
+ if (p != NULL)
+*p = 0;
+}
+
+// Test storage 0 as null pointer.
+// CHECK-LABEL: test_storage_null_pointer
+// CHECK: store i8* addrspacecast (i8 addrspace(4)* null to i8*), i8* addrspace(4)* %arg_private
+// CHECK: store i8 addrspace(3)* addrspacecast (i8 addrspace(4)* null to i8 addrspace(3)*), i8 addrspace(3)* addrspace(4)* %arg_local
+// CHECK: store i8 addrspace(1)* null, i8 addrspace(1)* addrspace(4)* %arg_global
+// CHECK: store i8 addrspace(2)* null, i8 addrspace(2)* addrspace(4)* %arg_constant
+// CHECK: store i8 addrspace(4)* null, i8 addrspace(4)* addrspace(4)* %arg_generic
+void test_storage_null_pointer(private char** arg_private,
+ local char** arg_local,
+ global char** arg_global,
+ constant char** arg_constant,
+ generic char** arg_generic) {
+
[PATCH] D26196: AMDGPU: Translate null pointers in private and local addr space
arsenm added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:6957-6958 + + llvm::Constant *translateNullPtr(const CodeGen::CodeGenModule , + llvm::Constant *C) const override; }; I was thinking that addrspacecast (generic null) should be valid for all targets, but I guess this saves the trouble of needing to fold out the nulls for the address spaces where null is supposed to be 0 Comment at: lib/CodeGen/TargetInfo.cpp:7038 + auto AS = PT->getAddressSpace(); + if (CGM.getTarget().getTriple().getArch() != llvm::Triple::amdgcn || +(AS != Ctx.getTargetAddressSpace(LangAS::opencl_local) && AS != 0)) The amdgcn check should be unnecessary Comment at: lib/CodeGen/TargetInfo.cpp:7039 + if (CGM.getTarget().getTriple().getArch() != llvm::Triple::amdgcn || +(AS != Ctx.getTargetAddressSpace(LangAS::opencl_local) && AS != 0)) +return C; Shouldn't the 0 be checking lang as::opencl_private? https://reviews.llvm.org/D26196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26196: AMDGPU: Translate null pointers in private and local addr space
arsenm added inline comments.
Comment at: test/CodeGenOpenCL/amdgpu-nullptr.cl:30
+
+// CHECK: icmp eq i8 addrspace(1)* %p, null
+void cmp_global(global char* p) {
Missing check-label
https://reviews.llvm.org/D26196
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26196: AMDGPU: Translate null pointers in private and local addr space
arsenm added inline comments. Comment at: test/CodeGenOpenCL/amdgpu-nullptr.cl:49 +} + I think there need to be a lot more tests. A few I can think of: - Tests that compare to NULL instead of 0 - Pointer compares without the explicit 0, i.e. if (p)/if(!p) - Storage of a pointer value - Calls with a pointer argument - Set of tests with constant address space pointers - Cast null to integer - Compare of null with null - Compare null in one address space with null in another https://reviews.llvm.org/D26196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26196: AMDGPU: Translate null pointers in private and local addr space
yaxunl created this revision.
yaxunl added reviewers: arsenm, tstellarAMD, rjmccall.
yaxunl added a subscriber: cfe-commits.
Herald added subscribers: tony-tye, nhaehnle, wdng, kzhuravl.
In amdgcn target, null pointers in global, constant, and generic address space
take value 0 but null pointers in private and local address space take value
-1. Currently LLVM assumes all null pointers take value 0, which results in
incorrectly translated IR. To workaround this issue, instead of emit null
pointers in local and private address space, a null pointer in generic address
space is emitted and casted to local and private address space.
A virtual member function translateNullPtr is added to TargetCodeGenInfo which
by default does nothing. Each target can override this virtual function for
translating null pointers.
A wrapper function translateNullPtr is added to CodegenModule to facilitate
performing the target specific translation of null pointers.
This change has no effect on other targets except amdgcn target.
https://reviews.llvm.org/D26196
Files:
lib/CodeGen/CGExprConstant.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenModule.h
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
test/CodeGenOpenCL/amdgpu-nullptr.cl
Index: test/CodeGenOpenCL/amdgpu-nullptr.cl
===
--- /dev/null
+++ test/CodeGenOpenCL/amdgpu-nullptr.cl
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -triple amdgcn -emit-llvm -o - | FileCheck %s
+
+// CHECK: @private_p = local_unnamed_addr addrspace(1) global i8* addrspacecast (i8 addrspace(4)* null to i8*), align 4
+private char *private_p = 0;
+
+// CHECK: @local_p = local_unnamed_addr addrspace(1) global i8 addrspace(3)* addrspacecast (i8 addrspace(4)* null to i8 addrspace(3)*), align 4
+local char *local_p = 0;
+
+// CHECK: @global_p = local_unnamed_addr addrspace(1) global i8 addrspace(1)* null, align 4
+global char *global_p = 0;
+
+// CHECK: @constant_p = local_unnamed_addr addrspace(1) global i8 addrspace(2)* null, align 4
+constant char *constant_p = 0;
+
+// CHECK: @generic_p = local_unnamed_addr addrspace(1) global i8 addrspace(4)* null, align 4
+generic char *generic_p = 0;
+
+// CHECK: icmp eq i8* %p, addrspacecast (i8 addrspace(4)* null to i8*)
+void cmp_private(private char* p) {
+ if (p != 0)
+*p = 0;
+}
+
+// CHECK: icmp eq i8 addrspace(3)* %p, addrspacecast (i8 addrspace(4)* null to i8 addrspace(3)*)
+void cmp_local(local char* p) {
+ if (p != 0)
+*p = 0;
+}
+
+// CHECK: icmp eq i8 addrspace(1)* %p, null
+void cmp_global(global char* p) {
+ if (p != 0)
+*p = 0;
+}
+
+// CHECK: icmp eq i8 addrspace(2)* %p, null
+char cmp_constant(constant char* p) {
+ if (p != 0)
+return *p;
+ else
+return 0;
+}
+
+// CHECK: icmp eq i8 addrspace(4)* %p, null
+void cmp_generic(generic char* p) {
+ if (p != 0)
+*p = 0;
+}
+
Index: lib/CodeGen/TargetInfo.h
===
--- lib/CodeGen/TargetInfo.h
+++ lib/CodeGen/TargetInfo.h
@@ -220,6 +220,13 @@
/// Get LLVM calling convention for OpenCL kernel.
virtual unsigned getOpenCLKernelCallingConv() const;
+
+ /// Translate null pointer to target specific value.
+ virtual llvm::Constant *translateNullPtr(const CodeGen::CodeGenModule ,
+ llvm::Constant *C) const {
+return C;
+ }
+
};
} // namespace CodeGen
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -6953,6 +6953,9 @@
void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) const override;
unsigned getOpenCLKernelCallingConv() const override;
+
+ llvm::Constant *translateNullPtr(const CodeGen::CodeGenModule ,
+ llvm::Constant *C) const override;
};
}
@@ -7018,6 +7021,30 @@
return llvm::CallingConv::AMDGPU_KERNEL;
}
+// In amdgcn target the null pointer in global, constant, and generic
+// address space has value 0 but in private and local address space has
+// value -1. Currently LLVM assumes null pointers always have value 0,
+// which results in incorrectly transformed IR. Therefore, instead of
+// emitting null pointers in private and local address spaces, a null
+// pointer in generic address space is emitted which is casted to a
+// pointer in local or private address space.
+llvm::Constant *AMDGPUTargetCodeGenInfo::translateNullPtr(
+const CodeGen::CodeGenModule , llvm::Constant *C) const {
+ if (!isa(C))
+return C;
+ auto PT = cast(C->getType());
+ auto = CGM.getContext();
+ auto AS = PT->getAddressSpace();
+ if (CGM.getTarget().getTriple().getArch() != llvm::Triple::amdgcn ||
+(AS != Ctx.getTargetAddressSpace(LangAS::opencl_local) && AS != 0))
+return C;
+
+ auto NPT = llvm::PointerType::get(PT->getElementType(),
+
