[PATCH] D32248: CodeGen: Cast alloca to expected address space
This revision was automatically updated to reflect the committed changes.
Closed by commit rL303370: CodeGen: Cast alloca to expected address space
(authored by yaxunl).
Changed prior to commit:
https://reviews.llvm.org/D32248?vs=99204=99479#toc
Repository:
rL LLVM
https://reviews.llvm.org/D32248
Files:
cfe/trunk/include/clang/AST/ASTContext.h
cfe/trunk/include/clang/AST/Type.h
cfe/trunk/include/clang/Basic/AddressSpaces.h
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/AST/ASTContext.cpp
cfe/trunk/lib/AST/TypePrinter.cpp
cfe/trunk/lib/Basic/Targets.cpp
cfe/trunk/lib/CodeGen/CGDecl.cpp
cfe/trunk/lib/CodeGen/CGExprScalar.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/lib/CodeGen/CodeGenTypeCache.h
cfe/trunk/lib/CodeGen/TargetInfo.cpp
cfe/trunk/lib/CodeGen/TargetInfo.h
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaType.cpp
cfe/trunk/test/CodeGen/address-space.c
cfe/trunk/test/CodeGenCXX/amdgcn-automatic-variable.cpp
cfe/trunk/test/CodeGenOpenCL/amdgcn-automatic-variable.cl
cfe/trunk/test/CodeGenOpenCL/amdgpu-alignment.cl
cfe/trunk/test/CodeGenOpenCL/amdgpu-debug-info-pointer-address-space.cl
cfe/trunk/test/CodeGenOpenCL/amdgpu-debug-info-variable-expression.cl
cfe/trunk/test/CodeGenOpenCL/amdgpu-nullptr.cl
cfe/trunk/test/CodeGenOpenCL/builtins-amdgcn.cl
cfe/trunk/test/CodeGenOpenCL/byval.cl
cfe/trunk/test/CodeGenOpenCL/size_t.cl
cfe/trunk/test/Sema/sizeof-struct-non-zero-as-member.cl
cfe/trunk/test/SemaOpenCL/storageclass-cl20.cl
cfe/trunk/test/SemaOpenCL/storageclass.cl
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2490,7 +2490,7 @@
def err_attribute_address_function_type : Error<
"function type may not be qualified with an address space">;
def err_as_qualified_auto_decl : Error<
- "automatic variable qualified with an address space">;
+ "automatic variable qualified with an%select{| invalid}0 address space">;
def err_arg_with_address_space : Error<
"parameter may not be qualified with an address space">;
def err_field_with_address_space : Error<
Index: cfe/trunk/include/clang/Basic/AddressSpaces.h
===
--- cfe/trunk/include/clang/Basic/AddressSpaces.h
+++ cfe/trunk/include/clang/Basic/AddressSpaces.h
@@ -45,13 +45,12 @@
// This denotes the count of language-specific address spaces and also
// the offset added to the target-specific address spaces, which are usually
// specified by address space attributes __attribute__(address_space(n))).
- Count
+ FirstTargetAddressSpace
};
/// The type of a lookup table which maps from language-specific address spaces
/// to target-specific ones.
-typedef unsigned Map[Count];
-
+typedef unsigned Map[FirstTargetAddressSpace];
}
}
Index: cfe/trunk/include/clang/AST/ASTContext.h
===
--- cfe/trunk/include/clang/AST/ASTContext.h
+++ cfe/trunk/include/clang/AST/ASTContext.h
@@ -2324,8 +2324,7 @@
uint64_t getTargetNullPointerValue(QualType QT) const;
bool addressSpaceMapManglingFor(unsigned AS) const {
-return AddrSpaceMapMangling ||
- AS >= LangAS::Count;
+return AddrSpaceMapMangling || AS >= LangAS::FirstTargetAddressSpace;
}
private:
Index: cfe/trunk/include/clang/AST/Type.h
===
--- cfe/trunk/include/clang/AST/Type.h
+++ cfe/trunk/include/clang/AST/Type.h
@@ -333,15 +333,18 @@
bool hasAddressSpace() const { return Mask & AddressSpaceMask; }
unsigned getAddressSpace() const { return Mask >> AddressSpaceShift; }
+ bool hasTargetSpecificAddressSpace() const {
+return getAddressSpace() >= LangAS::FirstTargetAddressSpace;
+ }
/// Get the address space attribute value to be printed by diagnostics.
unsigned getAddressSpaceAttributePrintValue() const {
auto Addr = getAddressSpace();
// This function is not supposed to be used with language specific
// address spaces. If that happens, the diagnostic message should consider
// printing the QualType instead of the address space value.
-assert(Addr == 0 || Addr >= LangAS::Count);
+assert(Addr == 0 || hasTargetSpecificAddressSpace());
if (Addr)
- return Addr - LangAS::Count;
+ return Addr - LangAS::FirstTargetAddressSpace;
// TODO: The diagnostic messages where Addr may be 0 should be fixed
// since it cannot differentiate the situation where 0 denotes the default
// address space or user specified __attribute__((address_space(0))).
Index: cfe/trunk/test/CodeGenCXX/amdgcn-automatic-variable.cpp
===
---
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl added a comment. Any further comments? Thanks. https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Looks great, thank you for being patient. https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:7294 llvm::PointerType *T, QualType QT) const override; + }; yaxunl wrote: > rjmccall wrote: > > Spurious newline? > will remove it. Sorry this line is needed to separate it from the next function. https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl updated this revision to Diff 99204.
yaxunl marked 4 inline comments as done.
yaxunl added a comment.
Herald added a subscriber: nhaehnle.
Removed special handling of OpenCL from getTargetAddressSpace.
https://reviews.llvm.org/D32248
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
include/clang/Basic/AddressSpaces.h
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/ASTContext.cpp
lib/AST/TypePrinter.cpp
lib/Basic/Targets.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/CodeGenTypeCache.h
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Sema/SemaDecl.cpp
lib/Sema/SemaType.cpp
test/CodeGen/address-space.c
test/CodeGenCXX/amdgcn-automatic-variable.cpp
test/CodeGenOpenCL/amdgcn-automatic-variable.cl
test/CodeGenOpenCL/amdgpu-alignment.cl
test/CodeGenOpenCL/amdgpu-debug-info-pointer-address-space.cl
test/CodeGenOpenCL/amdgpu-debug-info-variable-expression.cl
test/CodeGenOpenCL/amdgpu-nullptr.cl
test/CodeGenOpenCL/builtins-amdgcn.cl
test/CodeGenOpenCL/byval.cl
test/CodeGenOpenCL/size_t.cl
test/Sema/sizeof-struct-non-zero-as-member.cl
test/SemaOpenCL/storageclass-cl20.cl
test/SemaOpenCL/storageclass.cl
Index: test/SemaOpenCL/storageclass.cl
===
--- test/SemaOpenCL/storageclass.cl
+++ test/SemaOpenCL/storageclass.cl
@@ -13,29 +13,37 @@
constant int L1 = 0;
local int L2;
- auto int L3 = 7; // expected-error{{OpenCL version 1.2 does not support the 'auto' storage class specifier}}
- global int L4; // expected-error{{function scope variable cannot be declared in global address space}}
-
- constant int L5 = x; // expected-error {{initializer element is not a compile-time constant}}
- global int *constant L6 =
- private int *constant L7 = // expected-error {{initializer element is not a compile-time constant}}
- constant int *constant L8 =
- local int *constant L9 = // expected-error {{initializer element is not a compile-time constant}}
+ auto int L3 = 7;// expected-error{{OpenCL version 1.2 does not support the 'auto' storage class specifier}}
+ global int L4; // expected-error{{function scope variable cannot be declared in global address space}}
+ __attribute__((address_space(100))) int L5; // expected-error{{automatic variable qualified with an invalid address space}}
+
+ constant int L6 = x;// expected-error {{initializer element is not a compile-time constant}}
+ global int *constant L7 =
+ private int *constant L8 = // expected-error {{initializer element is not a compile-time constant}}
+ constant int *constant L9 =
+ local int *constant L10 = // expected-error {{initializer element is not a compile-time constant}}
}
static void kernel bar() { // expected-error{{kernel functions cannot be declared static}}
}
void f() {
- constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
- local int L2;// expected-error{{non-kernel function variable cannot be declared in local address space}}
+ constant int L1 = 0;// expected-error{{non-kernel function variable cannot be declared in constant address space}}
+ local int L2; // expected-error{{non-kernel function variable cannot be declared in local address space}}
+ global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
+ __attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
+
{
-constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
-local int L2;// expected-error{{non-kernel function variable cannot be declared in local address space}}
+constant int L1 = 0;// expected-error{{non-kernel function variable cannot be declared in constant address space}}
+local int L2; // expected-error{{non-kernel function variable cannot be declared in local address space}}
+global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
+__attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
}
- global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
- extern constant float L4;
- extern local float L5; // expected-error{{extern variable must reside in constant address space}}
- static int L6 = 0; // expected-error{{variables in function scope cannot be declared static}}
- static int L7; // expected-error{{variables in function
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl marked 14 inline comments as done.
yaxunl added inline comments.
Comment at: lib/CodeGen/CodeGenTypes.cpp:95
-
/// isRecordLayoutComplete - Return true if the specified type is already
Anastasia wrote:
> Why this change?
Will undo it.
Comment at: lib/CodeGen/TargetInfo.cpp:7294
llvm::PointerType *T, QualType QT) const override;
+
};
rjmccall wrote:
> Spurious newline?
will remove it.
Comment at: lib/Sema/SemaDecl.cpp:7206
+assert(T.getAddressSpace() != LangAS::opencl_constant);
+if (T.getAddressSpace() == LangAS::opencl_global) {
+ Diag(NewVD->getLocation(), diag::err_opencl_function_variable)
Anastasia wrote:
> I think it was nicer that all OpenCL related changes were grouped in one spot
> under the same if statement. Even though it isn't quite done everywhere. But
> this change makes them even more scattered.
Will move them down.
Comment at: test/CodeGenCXX/amdgcn-automatic-variable.cpp:25
+ // CHECK: store i32 2, i32* %[[r1]]
+ int lv1;
+ lv1 = 1;
Anastasia wrote:
> yaxunl wrote:
> > yaxunl wrote:
> > > Anastasia wrote:
> > > > I am wondering if all these different test cases are really needed. Are
> > > > we testing any different program paths from the patch?
> > > >
> > > > Also would it make sense to add a test case with an object in an AS
> > > > different to 0 (i.e. with `__attribute__((address_space(n)))`)
> > > I think at least I should cover the typical use cases of auto var.
> > >
> > > I will add a test for __attribute__((address_space(n
> > Sorry. I just checked that C++ does not allow
> > __attribute__((address_space(n))) on automatic var.
> I thought address space was undocumented extension. So essentially you can't
> have a cast to any address space but a default here? Would it work for the C
> instead to add `attribute((address_space(n)))`?
As John pointed out, this due to the embedded C specification (ISO/IEC TR
18037). Clang applies it to all languages except OpenCL.
Comment at: test/SemaOpenCL/storageclass-cl20.cl:16
+ global int L3; // expected-error{{function
scope variable cannot be declared in global address space}}
+ __attribute__((address_space(100))) int L4; // expected-error{{automatic
variable qualified with an invalid address space}}
Anastasia wrote:
> I am guessing generic should be rejected here as well... although it should
> only apply to pointers. But it seems we don't handle this properly.
will add a line for generic.
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall added inline comments.
Comment at: lib/CodeGen/TargetInfo.cpp:411
+CodeGen::CodeGenFunction , llvm::Value *Src, unsigned SrcAddr,
+unsigned DestAddr, llvm::Type *DestTy, bool isNonNull) const {
// Since target may map different address spaces in AST to the same address
Anastasia wrote:
> Seems like some parameters are unused.
This is just the default implementation. The idea is that targets that need to
do something more complex on a particular conversion — e.g. to make sure that
null pointers are translated correctly when they have different bit-patterns —
can easily do so.
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
Anastasia added inline comments.
Comment at: lib/CodeGen/CodeGenTypes.cpp:95
-
/// isRecordLayoutComplete - Return true if the specified type is already
Why this change?
Comment at: lib/CodeGen/TargetInfo.cpp:411
+CodeGen::CodeGenFunction , llvm::Value *Src, unsigned SrcAddr,
+unsigned DestAddr, llvm::Type *DestTy, bool isNonNull) const {
// Since target may map different address spaces in AST to the same address
Seems like some parameters are unused.
Comment at: lib/Sema/SemaDecl.cpp:7206
+assert(T.getAddressSpace() != LangAS::opencl_constant);
+if (T.getAddressSpace() == LangAS::opencl_global) {
+ Diag(NewVD->getLocation(), diag::err_opencl_function_variable)
I think it was nicer that all OpenCL related changes were grouped in one spot
under the same if statement. Even though it isn't quite done everywhere. But
this change makes them even more scattered.
Comment at: test/CodeGenCXX/amdgcn-automatic-variable.cpp:25
+ // CHECK: store i32 2, i32* %[[r1]]
+ int lv1;
+ lv1 = 1;
yaxunl wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > I am wondering if all these different test cases are really needed. Are
> > > we testing any different program paths from the patch?
> > >
> > > Also would it make sense to add a test case with an object in an AS
> > > different to 0 (i.e. with `__attribute__((address_space(n)))`)
> > I think at least I should cover the typical use cases of auto var.
> >
> > I will add a test for __attribute__((address_space(n
> Sorry. I just checked that C++ does not allow
> __attribute__((address_space(n))) on automatic var.
I thought address space was undocumented extension. So essentially you can't
have a cast to any address space but a default here? Would it work for the C
instead to add `attribute((address_space(n)))`?
Comment at: test/SemaOpenCL/storageclass-cl20.cl:16
+ global int L3; // expected-error{{function
scope variable cannot be declared in global address space}}
+ __attribute__((address_space(100))) int L4; // expected-error{{automatic
variable qualified with an invalid address space}}
I am guessing generic should be rejected here as well... although it should
only apply to pointers. But it seems we don't handle this properly.
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall added inline comments.
Comment at: lib/CodeGen/TargetInfo.cpp:7296
+ unsigned getASTAllocaAddressSpace() const override {
+return LangAS::Count + getABIInfo().getDataLayout().getAllocaAddrSpace();
+ }
yaxunl wrote:
> rjmccall wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > Can we rename LangAS::Count to something more meaningful like
> > > > LangAS::FirstTargetAddressSpace? I think this would clarify a lot of
> > > > code.
> > > >
> > > > Is the OpenCL special case in ASTContext::getTargetAddressSpace still
> > > > correct with this patch? A pointer in LangAS::Default should be
> > > > lowered as a generic pointer instead of a pointer into the alloca
> > > > address space, right?
> > > Will do.
> > >
> > > The OpenCL special case in ASTContext::getTargetAddressSpace is still
> > > correct with this patch. In OpenCL private address space is still
> > > represented in AST by LangAS::Default, so a pointer in LangAS::Default
> > > should be lowered as a pointer to alloca address space.
> > Then I don't understand. If __private is always the alloca address space,
> > why does IRGen have to add addrspace casts after allocating local variables
> > in it? IRGen's invariant is that the value stored in LocalDeclMap for a
> > variable of AST type T is a value of type [[T]] addrspace([[AS]]) *, where
> > [[T]] is the lowered IR type for T and [[AS]] is the target address space
> > of the variable. For auto variables, AS is always LangAS::Default, which
> > according to you means that [[AS]] is always LLVM's notion of the alloca
> > address space, which is exactly the type produced by alloca. So why is
> > there ever a cast?
> >
> > My understanding was that, on your target, __private is (at least
> > sometimes) a 64-bit extension of the alloca address space, which is 32-bit.
> > But that makes __private a different address space from the alloca address
> > space, so the special case in getTargetAddressSpace is still wrong, because
> > that special case means that a pointer-to-__private type will be lowered to
> > be a 32-bit pointer type.
> >
> > Also, I'm not sure why the normal AddrSpaceMap mechanism isn't sufficient
> > for mapping LangAS::Default.
> This work is mostly for C++.
>
> For OpenCL, the default address space is mapped to alloca address space (5),
> there is no address space cast inserted.
>
> For C++, the default address space is mapped to generic address space (0),
> therefore the address space cast is needed.
Wait, I think I might understand. You're saying that, when you're compiling
OpenCL, you want __private to just be the normal 32-bit address space, but when
you're compiling other languages, you want LangAS::Default to be this 64-bit
extension, probably because those other languages don't break things down in
address spaces as consistently as OpenCL and so it's more important for
LangAS::Default to be able to reach across address spaces.
I do not feel that it's correct for this to be hard-coded in ASTContext; it
really seems like a target-specific policy that should just be reflected in
AddrSpaceMap. That does mean that you'll have to find some way of informing
your TargetInfo that it's building OpenCL. The normal design for something
like that would be a target option that the driver would automatically set when
it recognized that it was building OpenCL; or you could change
getAddressSpaceMap to take a LangOptions.
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl added inline comments.
Comment at: lib/CodeGen/TargetInfo.cpp:7296
+ unsigned getASTAllocaAddressSpace() const override {
+return LangAS::Count + getABIInfo().getDataLayout().getAllocaAddrSpace();
+ }
rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > Can we rename LangAS::Count to something more meaningful like
> > > LangAS::FirstTargetAddressSpace? I think this would clarify a lot of
> > > code.
> > >
> > > Is the OpenCL special case in ASTContext::getTargetAddressSpace still
> > > correct with this patch? A pointer in LangAS::Default should be lowered
> > > as a generic pointer instead of a pointer into the alloca address space,
> > > right?
> > Will do.
> >
> > The OpenCL special case in ASTContext::getTargetAddressSpace is still
> > correct with this patch. In OpenCL private address space is still
> > represented in AST by LangAS::Default, so a pointer in LangAS::Default
> > should be lowered as a pointer to alloca address space.
> Then I don't understand. If __private is always the alloca address space,
> why does IRGen have to add addrspace casts after allocating local variables
> in it? IRGen's invariant is that the value stored in LocalDeclMap for a
> variable of AST type T is a value of type [[T]] addrspace([[AS]]) *, where
> [[T]] is the lowered IR type for T and [[AS]] is the target address space of
> the variable. For auto variables, AS is always LangAS::Default, which
> according to you means that [[AS]] is always LLVM's notion of the alloca
> address space, which is exactly the type produced by alloca. So why is there
> ever a cast?
>
> My understanding was that, on your target, __private is (at least sometimes)
> a 64-bit extension of the alloca address space, which is 32-bit. But that
> makes __private a different address space from the alloca address space, so
> the special case in getTargetAddressSpace is still wrong, because that
> special case means that a pointer-to-__private type will be lowered to be a
> 32-bit pointer type.
>
> Also, I'm not sure why the normal AddrSpaceMap mechanism isn't sufficient for
> mapping LangAS::Default.
This work is mostly for C++.
For OpenCL, the default address space is mapped to alloca address space (5),
there is no address space cast inserted.
For C++, the default address space is mapped to generic address space (0),
therefore the address space cast is needed.
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall added inline comments.
Comment at: lib/CodeGen/TargetInfo.cpp:7296
+ unsigned getASTAllocaAddressSpace() const override {
+return LangAS::Count + getABIInfo().getDataLayout().getAllocaAddrSpace();
+ }
yaxunl wrote:
> rjmccall wrote:
> > Can we rename LangAS::Count to something more meaningful like
> > LangAS::FirstTargetAddressSpace? I think this would clarify a lot of code.
> >
> > Is the OpenCL special case in ASTContext::getTargetAddressSpace still
> > correct with this patch? A pointer in LangAS::Default should be lowered as
> > a generic pointer instead of a pointer into the alloca address space, right?
> Will do.
>
> The OpenCL special case in ASTContext::getTargetAddressSpace is still correct
> with this patch. In OpenCL private address space is still represented in AST
> by LangAS::Default, so a pointer in LangAS::Default should be lowered as a
> pointer to alloca address space.
Then I don't understand. If __private is always the alloca address space, why
does IRGen have to add addrspace casts after allocating local variables in it?
IRGen's invariant is that the value stored in LocalDeclMap for a variable of
AST type T is a value of type [[T]] addrspace([[AS]]) *, where [[T]] is the
lowered IR type for T and [[AS]] is the target address space of the variable.
For auto variables, AS is always LangAS::Default, which according to you means
that [[AS]] is always LLVM's notion of the alloca address space, which is
exactly the type produced by alloca. So why is there ever a cast?
My understanding was that, on your target, __private is (at least sometimes) a
64-bit extension of the alloca address space, which is 32-bit. But that makes
__private a different address space from the alloca address space, so the
special case in getTargetAddressSpace is still wrong, because that special case
means that a pointer-to-__private type will be lowered to be a 32-bit pointer
type.
Also, I'm not sure why the normal AddrSpaceMap mechanism isn't sufficient for
mapping LangAS::Default.
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl updated this revision to Diff 99149.
yaxunl marked an inline comment as done.
yaxunl added a comment.
Revised by John's comments.
https://reviews.llvm.org/D32248
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
include/clang/Basic/AddressSpaces.h
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/ASTContext.cpp
lib/AST/TypePrinter.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/CodeGenTypeCache.h
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Sema/SemaDecl.cpp
lib/Sema/SemaType.cpp
test/CodeGen/address-space.c
test/CodeGenCXX/amdgcn-automatic-variable.cpp
test/CodeGenOpenCL/amdgcn-automatic-variable.cl
test/SemaOpenCL/storageclass-cl20.cl
test/SemaOpenCL/storageclass.cl
Index: test/SemaOpenCL/storageclass.cl
===
--- test/SemaOpenCL/storageclass.cl
+++ test/SemaOpenCL/storageclass.cl
@@ -13,29 +13,37 @@
constant int L1 = 0;
local int L2;
- auto int L3 = 7; // expected-error{{OpenCL version 1.2 does not support the 'auto' storage class specifier}}
- global int L4; // expected-error{{function scope variable cannot be declared in global address space}}
-
- constant int L5 = x; // expected-error {{initializer element is not a compile-time constant}}
- global int *constant L6 =
- private int *constant L7 = // expected-error {{initializer element is not a compile-time constant}}
- constant int *constant L8 =
- local int *constant L9 = // expected-error {{initializer element is not a compile-time constant}}
+ auto int L3 = 7;// expected-error{{OpenCL version 1.2 does not support the 'auto' storage class specifier}}
+ global int L4; // expected-error{{function scope variable cannot be declared in global address space}}
+ __attribute__((address_space(100))) int L5; // expected-error{{automatic variable qualified with an invalid address space}}
+
+ constant int L6 = x;// expected-error {{initializer element is not a compile-time constant}}
+ global int *constant L7 =
+ private int *constant L8 = // expected-error {{initializer element is not a compile-time constant}}
+ constant int *constant L9 =
+ local int *constant L10 = // expected-error {{initializer element is not a compile-time constant}}
}
static void kernel bar() { // expected-error{{kernel functions cannot be declared static}}
}
void f() {
- constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
- local int L2;// expected-error{{non-kernel function variable cannot be declared in local address space}}
+ constant int L1 = 0;// expected-error{{non-kernel function variable cannot be declared in constant address space}}
+ local int L2; // expected-error{{non-kernel function variable cannot be declared in local address space}}
+ global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
+ __attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
+
{
-constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
-local int L2;// expected-error{{non-kernel function variable cannot be declared in local address space}}
+constant int L1 = 0;// expected-error{{non-kernel function variable cannot be declared in constant address space}}
+local int L2; // expected-error{{non-kernel function variable cannot be declared in local address space}}
+global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
+__attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
}
- global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
- extern constant float L4;
- extern local float L5; // expected-error{{extern variable must reside in constant address space}}
- static int L6 = 0; // expected-error{{variables in function scope cannot be declared static}}
- static int L7; // expected-error{{variables in function scope cannot be declared static}}
+
+
+ extern constant float L5;
+ extern local float L6; // expected-error{{extern variable must reside in constant address space}}
+
+ static int L7 = 0; // expected-error{{variables in function scope cannot be declared static}}
+ static int L8; // expected-error{{variables in function scope cannot be declared static}}
}
Index: test/SemaOpenCL/storageclass-cl20.cl
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl marked 13 inline comments as done.
yaxunl added inline comments.
Comment at: lib/CodeGen/TargetInfo.cpp:7296
+ unsigned getASTAllocaAddressSpace() const override {
+return LangAS::Count + getABIInfo().getDataLayout().getAllocaAddrSpace();
+ }
rjmccall wrote:
> Can we rename LangAS::Count to something more meaningful like
> LangAS::FirstTargetAddressSpace? I think this would clarify a lot of code.
>
> Is the OpenCL special case in ASTContext::getTargetAddressSpace still correct
> with this patch? A pointer in LangAS::Default should be lowered as a generic
> pointer instead of a pointer into the alloca address space, right?
Will do.
The OpenCL special case in ASTContext::getTargetAddressSpace is still correct
with this patch. In OpenCL private address space is still represented in AST by
LangAS::Default, so a pointer in LangAS::Default should be lowered as a pointer
to alloca address space.
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:1120
+address.getPointer()->getType()->getPointerElementType()->getPointerTo(
+getContext().getTargetAddressSpace(T.getAddressSpace())),
+/*non-null*/ true);
A lot of this line can be address.getElementType().
Comment at: lib/CodeGen/CodeGenTypes.cpp:95
-
/// isRecordLayoutComplete - Return true if the specified type is already
Did you intend to change this file?
Comment at: lib/CodeGen/TargetInfo.cpp:7296
+ unsigned getASTAllocaAddressSpace() const override {
+return LangAS::Count + getABIInfo().getDataLayout().getAllocaAddrSpace();
+ }
Can we rename LangAS::Count to something more meaningful like
LangAS::FirstTargetAddressSpace? I think this would clarify a lot of code.
Is the OpenCL special case in ASTContext::getTargetAddressSpace still correct
with this patch? A pointer in LangAS::Default should be lowered as a generic
pointer instead of a pointer into the alloca address space, right?
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl updated this revision to Diff 99099.
yaxunl added a comment.
Revised by John's comments.
https://reviews.llvm.org/D32248
Files:
include/clang/AST/Type.h
include/clang/Basic/DiagnosticSemaKinds.td
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/CodeGenTypeCache.h
lib/CodeGen/CodeGenTypes.cpp
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Sema/SemaDecl.cpp
test/CodeGen/address-space.c
test/CodeGenCXX/amdgcn-automatic-variable.cpp
test/CodeGenOpenCL/amdgcn-automatic-variable.cl
test/SemaOpenCL/storageclass-cl20.cl
test/SemaOpenCL/storageclass.cl
Index: test/SemaOpenCL/storageclass.cl
===
--- test/SemaOpenCL/storageclass.cl
+++ test/SemaOpenCL/storageclass.cl
@@ -13,29 +13,37 @@
constant int L1 = 0;
local int L2;
- auto int L3 = 7; // expected-error{{OpenCL version 1.2 does not support the 'auto' storage class specifier}}
- global int L4; // expected-error{{function scope variable cannot be declared in global address space}}
-
- constant int L5 = x; // expected-error {{initializer element is not a compile-time constant}}
- global int *constant L6 =
- private int *constant L7 = // expected-error {{initializer element is not a compile-time constant}}
- constant int *constant L8 =
- local int *constant L9 = // expected-error {{initializer element is not a compile-time constant}}
+ auto int L3 = 7;// expected-error{{OpenCL version 1.2 does not support the 'auto' storage class specifier}}
+ global int L4; // expected-error{{function scope variable cannot be declared in global address space}}
+ __attribute__((address_space(100))) int L5; // expected-error{{automatic variable qualified with an invalid address space}}
+
+ constant int L6 = x;// expected-error {{initializer element is not a compile-time constant}}
+ global int *constant L7 =
+ private int *constant L8 = // expected-error {{initializer element is not a compile-time constant}}
+ constant int *constant L9 =
+ local int *constant L10 = // expected-error {{initializer element is not a compile-time constant}}
}
static void kernel bar() { // expected-error{{kernel functions cannot be declared static}}
}
void f() {
- constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
- local int L2;// expected-error{{non-kernel function variable cannot be declared in local address space}}
+ constant int L1 = 0;// expected-error{{non-kernel function variable cannot be declared in constant address space}}
+ local int L2; // expected-error{{non-kernel function variable cannot be declared in local address space}}
+ global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
+ __attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
+
{
-constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
-local int L2;// expected-error{{non-kernel function variable cannot be declared in local address space}}
+constant int L1 = 0;// expected-error{{non-kernel function variable cannot be declared in constant address space}}
+local int L2; // expected-error{{non-kernel function variable cannot be declared in local address space}}
+global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
+__attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
}
- global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
- extern constant float L4;
- extern local float L5; // expected-error{{extern variable must reside in constant address space}}
- static int L6 = 0; // expected-error{{variables in function scope cannot be declared static}}
- static int L7; // expected-error{{variables in function scope cannot be declared static}}
+
+
+ extern constant float L5;
+ extern local float L6; // expected-error{{extern variable must reside in constant address space}}
+
+ static int L7 = 0; // expected-error{{variables in function scope cannot be declared static}}
+ static int L8; // expected-error{{variables in function scope cannot be declared static}}
}
Index: test/SemaOpenCL/storageclass-cl20.cl
===
--- test/SemaOpenCL/storageclass-cl20.cl
+++ test/SemaOpenCL/storageclass-cl20.cl
@@ -12,7 +12,8 @@
constant int L1
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1119 + T.getAddressSpace(), address.getPointer()->getType()-> + getPointerElementType()->getPointerTo(getContext().getTargetAddressSpace( + T.getAddressSpace())), true); rjmccall wrote: > Hmm. I would prefer if we could avoid doing this in the fast path. > Suggestion: > > - Please add a variable (ASTAllocaAddressSpace) to CodeGenTypeCache that > stores the AST address space of the stack. You can add a function to > CodeGenTargetInfo to initialize this; in most targets, it will just return 0, > but your target can make it ~0U or something, anything works as long as it > doesn't incorrectly match an actual target address space. (But you should > really consider adding an actual target-specific address space for the stack! > It'll probably be useful in builtin code or something eventually.) > > - Please add an assert that T.getAddressSpace() == 0, and then make this > call conditional on whether ASTAllocaAddressSpace is non-zero. > > You can just write address.getElementType() instead of > address.getPointer()->getType()->getPointerElementType(). > > Are you intentionally leaving emission.Addr as the unconverted pointer? That > will probably mess everything else downstream that uses 'emission', and the > only reason you're not seeing that is that OpenCL probably doesn't actually > have anything downstream that uses 'emission' — destructors or whatever else. > > Please add an inline comment like >, /*non-null*/ true); Sorry I missed this comment. I will do the changes. About emission.Addr, I leave the unconverted pointer intentionally. I need this to work for both C++ and OpenCL. I checked the usage of emission.Addr and they seem to be only load or store, so I think it is safe to use the unconverted pointer. Now you mention destructor. I think I need to change that and I am going to add a lit test also. https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl updated this revision to Diff 99094.
yaxunl added a comment.
Fix format.
https://reviews.llvm.org/D32248
Files:
include/clang/AST/Type.h
include/clang/Basic/DiagnosticSemaKinds.td
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenTypes.cpp
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Sema/SemaDecl.cpp
test/CodeGen/address-space.c
test/CodeGenCXX/amdgcn-automatic-variable.cpp
test/CodeGenOpenCL/amdgcn-automatic-variable.cl
test/SemaOpenCL/storageclass-cl20.cl
test/SemaOpenCL/storageclass.cl
Index: test/SemaOpenCL/storageclass.cl
===
--- test/SemaOpenCL/storageclass.cl
+++ test/SemaOpenCL/storageclass.cl
@@ -13,29 +13,37 @@
constant int L1 = 0;
local int L2;
- auto int L3 = 7; // expected-error{{OpenCL version 1.2 does not support the 'auto' storage class specifier}}
- global int L4; // expected-error{{function scope variable cannot be declared in global address space}}
-
- constant int L5 = x; // expected-error {{initializer element is not a compile-time constant}}
- global int *constant L6 =
- private int *constant L7 = // expected-error {{initializer element is not a compile-time constant}}
- constant int *constant L8 =
- local int *constant L9 = // expected-error {{initializer element is not a compile-time constant}}
+ auto int L3 = 7;// expected-error{{OpenCL version 1.2 does not support the 'auto' storage class specifier}}
+ global int L4; // expected-error{{function scope variable cannot be declared in global address space}}
+ __attribute__((address_space(100))) int L5; // expected-error{{automatic variable qualified with an invalid address space}}
+
+ constant int L6 = x;// expected-error {{initializer element is not a compile-time constant}}
+ global int *constant L7 =
+ private int *constant L8 = // expected-error {{initializer element is not a compile-time constant}}
+ constant int *constant L9 =
+ local int *constant L10 = // expected-error {{initializer element is not a compile-time constant}}
}
static void kernel bar() { // expected-error{{kernel functions cannot be declared static}}
}
void f() {
- constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
- local int L2;// expected-error{{non-kernel function variable cannot be declared in local address space}}
+ constant int L1 = 0;// expected-error{{non-kernel function variable cannot be declared in constant address space}}
+ local int L2; // expected-error{{non-kernel function variable cannot be declared in local address space}}
+ global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
+ __attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
+
{
-constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
-local int L2;// expected-error{{non-kernel function variable cannot be declared in local address space}}
+constant int L1 = 0;// expected-error{{non-kernel function variable cannot be declared in constant address space}}
+local int L2; // expected-error{{non-kernel function variable cannot be declared in local address space}}
+global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
+__attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
}
- global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
- extern constant float L4;
- extern local float L5; // expected-error{{extern variable must reside in constant address space}}
- static int L6 = 0; // expected-error{{variables in function scope cannot be declared static}}
- static int L7; // expected-error{{variables in function scope cannot be declared static}}
+
+
+ extern constant float L5;
+ extern local float L6; // expected-error{{extern variable must reside in constant address space}}
+
+ static int L7 = 0; // expected-error{{variables in function scope cannot be declared static}}
+ static int L8; // expected-error{{variables in function scope cannot be declared static}}
}
Index: test/SemaOpenCL/storageclass-cl20.cl
===
--- test/SemaOpenCL/storageclass-cl20.cl
+++ test/SemaOpenCL/storageclass-cl20.cl
@@ -12,7 +12,8 @@
constant int L1 = 0;
local int L2;
- global int L3; // expected-error{{function scope
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall added a comment.
Looking good. We had some overlapping patches and comments there, so I want to
make sure you didn't miss my comment on EmitAutoVarAlloca.
Comment at: lib/Sema/SemaDecl.cpp:7205
+}
+assert (T.getAddressSpace() != LangAS::opencl_constant);
+if (T.getAddressSpace() == LangAS::opencl_global) {
LLVM code style doesn't put a space after assert.
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl updated this revision to Diff 99061.
yaxunl added a comment.
Fix diagnostic for global addr
https://reviews.llvm.org/D32248
Files:
include/clang/AST/Type.h
include/clang/Basic/DiagnosticSemaKinds.td
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenTypes.cpp
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Sema/SemaDecl.cpp
test/CodeGen/address-space.c
test/CodeGenCXX/amdgcn-automatic-variable.cpp
test/CodeGenOpenCL/amdgcn-automatic-variable.cl
test/SemaOpenCL/storageclass-cl20.cl
test/SemaOpenCL/storageclass.cl
Index: test/SemaOpenCL/storageclass.cl
===
--- test/SemaOpenCL/storageclass.cl
+++ test/SemaOpenCL/storageclass.cl
@@ -13,29 +13,37 @@
constant int L1 = 0;
local int L2;
- auto int L3 = 7; // expected-error{{OpenCL version 1.2 does not support the 'auto' storage class specifier}}
- global int L4; // expected-error{{function scope variable cannot be declared in global address space}}
-
- constant int L5 = x; // expected-error {{initializer element is not a compile-time constant}}
- global int *constant L6 =
- private int *constant L7 = // expected-error {{initializer element is not a compile-time constant}}
- constant int *constant L8 =
- local int *constant L9 = // expected-error {{initializer element is not a compile-time constant}}
+ auto int L3 = 7;// expected-error{{OpenCL version 1.2 does not support the 'auto' storage class specifier}}
+ global int L4; // expected-error{{function scope variable cannot be declared in global address space}}
+ __attribute__((address_space(100))) int L5; // expected-error{{automatic variable qualified with an invalid address space}}
+
+ constant int L6 = x;// expected-error {{initializer element is not a compile-time constant}}
+ global int *constant L7 =
+ private int *constant L8 = // expected-error {{initializer element is not a compile-time constant}}
+ constant int *constant L9 =
+ local int *constant L10 = // expected-error {{initializer element is not a compile-time constant}}
}
static void kernel bar() { // expected-error{{kernel functions cannot be declared static}}
}
void f() {
- constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
- local int L2;// expected-error{{non-kernel function variable cannot be declared in local address space}}
+ constant int L1 = 0;// expected-error{{non-kernel function variable cannot be declared in constant address space}}
+ local int L2; // expected-error{{non-kernel function variable cannot be declared in local address space}}
+ global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
+ __attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
+
{
-constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
-local int L2;// expected-error{{non-kernel function variable cannot be declared in local address space}}
+constant int L1 = 0;// expected-error{{non-kernel function variable cannot be declared in constant address space}}
+local int L2; // expected-error{{non-kernel function variable cannot be declared in local address space}}
+global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
+__attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
}
- global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
- extern constant float L4;
- extern local float L5; // expected-error{{extern variable must reside in constant address space}}
- static int L6 = 0; // expected-error{{variables in function scope cannot be declared static}}
- static int L7; // expected-error{{variables in function scope cannot be declared static}}
+
+
+ extern constant float L5;
+ extern local float L6; // expected-error{{extern variable must reside in constant address space}}
+
+ static int L7 = 0; // expected-error{{variables in function scope cannot be declared static}}
+ static int L8; // expected-error{{variables in function scope cannot be declared static}}
}
Index: test/SemaOpenCL/storageclass-cl20.cl
===
--- test/SemaOpenCL/storageclass-cl20.cl
+++ test/SemaOpenCL/storageclass-cl20.cl
@@ -12,7 +12,8 @@
constant int L1 = 0;
local int L2;
- global int L3; //
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:7210 + return; +} } err_opencl_function_variable seems like a better diagnostic, at least for opencl_global. You can fall back on this more general diagnostic for other address spaces. I think you can just assert here that T.getAddressSpace() != LangAS::opencl_constant instead of checking it. Thinking about it, it feels like opencl_local should be handled the same way: they shouldn't get here because they don't really have automatic storage duration. They're much more like a thread-local than an actual local. However, we can tackle that some other time. https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl updated this revision to Diff 99056.
yaxunl marked an inline comment as done.
yaxunl added a comment.
diagnose automatic var with invalid addr space for OpenCL.
https://reviews.llvm.org/D32248
Files:
include/clang/AST/Type.h
include/clang/Basic/DiagnosticSemaKinds.td
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenTypes.cpp
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Sema/SemaDecl.cpp
test/CodeGen/address-space.c
test/CodeGenCXX/amdgcn-automatic-variable.cpp
test/CodeGenOpenCL/amdgcn-automatic-variable.cl
test/SemaOpenCL/atomic-init.cl
test/SemaOpenCL/storageclass-cl20.cl
test/SemaOpenCL/storageclass.cl
Index: test/SemaOpenCL/storageclass.cl
===
--- test/SemaOpenCL/storageclass.cl
+++ test/SemaOpenCL/storageclass.cl
@@ -13,29 +13,37 @@
constant int L1 = 0;
local int L2;
- auto int L3 = 7; // expected-error{{OpenCL version 1.2 does not support the 'auto' storage class specifier}}
- global int L4; // expected-error{{function scope variable cannot be declared in global address space}}
-
- constant int L5 = x; // expected-error {{initializer element is not a compile-time constant}}
- global int *constant L6 =
- private int *constant L7 = // expected-error {{initializer element is not a compile-time constant}}
- constant int *constant L8 =
- local int *constant L9 = // expected-error {{initializer element is not a compile-time constant}}
+ auto int L3 = 7;// expected-error{{OpenCL version 1.2 does not support the 'auto' storage class specifier}}
+ global int L4; // expected-error{{automatic variable qualified with an invalid address space}}
+ __attribute__((address_space(100))) int L5; // expected-error{{automatic variable qualified with an invalid address space}}
+
+ constant int L6 = x;// expected-error {{initializer element is not a compile-time constant}}
+ global int *constant L7 =
+ private int *constant L8 = // expected-error {{initializer element is not a compile-time constant}}
+ constant int *constant L9 =
+ local int *constant L10 = // expected-error {{initializer element is not a compile-time constant}}
}
static void kernel bar() { // expected-error{{kernel functions cannot be declared static}}
}
void f() {
- constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
- local int L2;// expected-error{{non-kernel function variable cannot be declared in local address space}}
+ constant int L1 = 0;// expected-error{{non-kernel function variable cannot be declared in constant address space}}
+ local int L2; // expected-error{{non-kernel function variable cannot be declared in local address space}}
+ global int L3; // expected-error{{automatic variable qualified with an invalid address space}}
+ __attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
+
{
-constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
-local int L2;// expected-error{{non-kernel function variable cannot be declared in local address space}}
+constant int L1 = 0;// expected-error{{non-kernel function variable cannot be declared in constant address space}}
+local int L2; // expected-error{{non-kernel function variable cannot be declared in local address space}}
+global int L3; // expected-error{{automatic variable qualified with an invalid address space}}
+__attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
}
- global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
- extern constant float L4;
- extern local float L5; // expected-error{{extern variable must reside in constant address space}}
- static int L6 = 0; // expected-error{{variables in function scope cannot be declared static}}
- static int L7; // expected-error{{variables in function scope cannot be declared static}}
+
+
+ extern constant float L5;
+ extern local float L6; // expected-error{{extern variable must reside in constant address space}}
+
+ static int L7 = 0; // expected-error{{variables in function scope cannot be declared static}}
+ static int L8; // expected-error{{variables in function scope cannot be declared static}}
}
Index: test/SemaOpenCL/storageclass-cl20.cl
===
--- test/SemaOpenCL/storageclass-cl20.cl
+++ test/SemaOpenCL/storageclass-cl20.cl
@@ -12,7 +12,8 @@
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1115 + assert(T.getAddressSpace() == LangAS::Default || + T.getQualifiers().hasTargetSpecificAddressSpace()); + auto Addr = getTargetHooks().performAddrSpaceCast(*this, yaxunl wrote: > t-tye wrote: > > Should allowing specifying an address space on a function local automatic > > variable in OpenCL be allowed? It seems generating LLVM IR that allocates > > the variable in the alloca address space then address space casting the > > pointer to some other address space is not what such a language feature is > > requesting. > > > > It seems it is really requesting that the variable is allocated in a > > different address space. That could be indicated by putting a different > > address space on the alloca itself, but the builder only allows an alloca > > to use the default alloca address space. No target supports this ability. > > > > So maybe the best solution is to make OpenCL be the same as C and not allow > > an address space qualifier on function scope automatic variables. > > > > If that is done then this assert will only allow the language default > > address space. > Agree. I think that is why C++ forbids `__attribute__((address_space(n))` on > automatic variables. I will make it apply to all languages. Yeah, the embedded C specification (ISO/IEC TR 18037) says "an address space name cannot be used to qualify an object that has automatic storage duration", so this restriction should be applied regardless of language mode. We can deal with the possibility of auto variables in other address spaces when someone wants to add that capability. (OpenCL's __constant is an acceptable exception because it implicitly makes the variable static.) Comment at: lib/CodeGen/CGDecl.cpp:1119 + T.getAddressSpace(), address.getPointer()->getType()-> + getPointerElementType()->getPointerTo(getContext().getTargetAddressSpace( + T.getAddressSpace())), true); Hmm. I would prefer if we could avoid doing this in the fast path. Suggestion: - Please add a variable (ASTAllocaAddressSpace) to CodeGenTypeCache that stores the AST address space of the stack. You can add a function to CodeGenTargetInfo to initialize this; in most targets, it will just return 0, but your target can make it ~0U or something, anything works as long as it doesn't incorrectly match an actual target address space. (But you should really consider adding an actual target-specific address space for the stack! It'll probably be useful in builtin code or something eventually.) - Please add an assert that T.getAddressSpace() == 0, and then make this call conditional on whether ASTAllocaAddressSpace is non-zero. You can just write address.getElementType() instead of address.getPointer()->getType()->getPointerElementType(). Are you intentionally leaving emission.Addr as the unconverted pointer? That will probably mess everything else downstream that uses 'emission', and the only reason you're not seeing that is that OpenCL probably doesn't actually have anything downstream that uses 'emission' — destructors or whatever else. Please add an inline comment like , /*non-null*/ true); Comment at: lib/CodeGen/TargetInfo.cpp:7294 llvm::PointerType *T, QualType QT) const override; + }; Spurious newline? https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl marked 2 inline comments as done. yaxunl added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1115 + assert(T.getAddressSpace() == LangAS::Default || + T.getQualifiers().hasTargetSpecificAddressSpace()); + auto Addr = getTargetHooks().performAddrSpaceCast(*this, t-tye wrote: > Should allowing specifying an address space on a function local automatic > variable in OpenCL be allowed? It seems generating LLVM IR that allocates the > variable in the alloca address space then address space casting the pointer > to some other address space is not what such a language feature is requesting. > > It seems it is really requesting that the variable is allocated in a > different address space. That could be indicated by putting a different > address space on the alloca itself, but the builder only allows an alloca to > use the default alloca address space. No target supports this ability. > > So maybe the best solution is to make OpenCL be the same as C and not allow > an address space qualifier on function scope automatic variables. > > If that is done then this assert will only allow the language default address > space. Agree. I think that is why C++ forbids `__attribute__((address_space(n))` on automatic variables. I will make it apply to all languages. https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
t-tye added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1115 + assert(T.getAddressSpace() == LangAS::Default || + T.getQualifiers().hasTargetSpecificAddressSpace()); + auto Addr = getTargetHooks().performAddrSpaceCast(*this, Should allowing specifying an address space on a function local automatic variable in OpenCL be allowed? It seems generating LLVM IR that allocates the variable in the alloca address space then address space casting the pointer to some other address space is not what such a language feature is requesting. It seems it is really requesting that the variable is allocated in a different address space. That could be indicated by putting a different address space on the alloca itself, but the builder only allows an alloca to use the default alloca address space. No target supports this ability. So maybe the best solution is to make OpenCL be the same as C and not allow an address space qualifier on function scope automatic variables. If that is done then this assert will only allow the language default address space. https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl updated this revision to Diff 99014.
yaxunl added a comment.
Removed adjustAddrSpaceForAutoVar and use performAddrSpaceCast instead.
https://reviews.llvm.org/D32248
Files:
include/clang/AST/Type.h
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenTypes.cpp
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
test/CodeGen/address-space.c
test/CodeGenCXX/amdgcn-automatic-variable.cpp
test/CodeGenOpenCL/amdgcn-automatic-variable.cl
Index: test/CodeGenOpenCL/amdgcn-automatic-variable.cl
===
--- /dev/null
+++ test/CodeGenOpenCL/amdgcn-automatic-variable.cl
@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 -O0 -cl-std=CL1.2 -triple amdgcn---amdgiz -emit-llvm %s -o - | FileCheck -check-prefixes=CHECK,CL12 %s
+// RUN: %clang_cc1 -O0 -cl-std=CL2.0 -triple amdgcn---amdgiz -emit-llvm %s -o - | FileCheck -check-prefixes=CHECK,CL20 %s
+
+// CL12-LABEL: define void @func1(i32 addrspace(5)* %x)
+// CL20-LABEL: define void @func1(i32* %x)
+void func1(int *x) {
+ // CL12: %[[x_addr:.*]] = alloca i32 addrspace(5)*{{.*}}addrspace(5)
+ // CL12: store i32 addrspace(5)* %x, i32 addrspace(5)* addrspace(5)* %[[x_addr]]
+ // CL12: %[[r0:.*]] = load i32 addrspace(5)*, i32 addrspace(5)* addrspace(5)* %[[x_addr]]
+ // CL12: store i32 1, i32 addrspace(5)* %[[r0]]
+ // CL20: %[[x_addr:.*]] = alloca i32*{{.*}}addrspace(5)
+ // CL20: store i32* %x, i32* addrspace(5)* %[[x_addr]]
+ // CL20: %[[r0:.*]] = load i32*, i32* addrspace(5)* %[[x_addr]]
+ // CL20: store i32 1, i32* %[[r0]]
+ *x = 1;
+}
+
+// CHECK-LABEL: define void @func2()
+void func2(void) {
+ // CHECK: %lv1 = alloca i32, align 4, addrspace(5)
+ // CHECK: %lv2 = alloca i32, align 4, addrspace(5)
+ // CHECK: %la = alloca [100 x i32], align 4, addrspace(5)
+ // CL12: %lp1 = alloca i32 addrspace(5)*, align 4, addrspace(5)
+ // CL12: %lp2 = alloca i32 addrspace(5)*, align 4, addrspace(5)
+ // CL20: %lp1 = alloca i32*, align 4, addrspace(5)
+ // CL20: %lp2 = alloca i32*, align 4, addrspace(5)
+ // CHECK: %lvc = alloca i32, align 4, addrspace(5)
+
+ // CHECK: store i32 1, i32 addrspace(5)* %lv1
+ int lv1;
+ lv1 = 1;
+ // CHECK: store i32 2, i32 addrspace(5)* %lv2
+ int lv2 = 2;
+
+ // CHECK: %[[arrayidx:.*]] = getelementptr inbounds [100 x i32], [100 x i32] addrspace(5)* %la, i64 0, i64 0
+ // CHECK: store i32 3, i32 addrspace(5)* %[[arrayidx]], align 4
+ int la[100];
+ la[0] = 3;
+
+ // CL12: store i32 addrspace(5)* %lv1, i32 addrspace(5)* addrspace(5)* %lp1, align 4
+ // CL20: %[[r0:.*]] = addrspacecast i32 addrspace(5)* %lv1 to i32*
+ // CL20: store i32* %[[r0]], i32* addrspace(5)* %lp1, align 4
+ int *lp1 =
+
+ // CHECK: %[[arraydecay:.*]] = getelementptr inbounds [100 x i32], [100 x i32] addrspace(5)* %la, i32 0, i32 0
+ // CL12: store i32 addrspace(5)* %[[arraydecay]], i32 addrspace(5)* addrspace(5)* %lp2, align 4
+ // CL20: %[[r1:.*]] = addrspacecast i32 addrspace(5)* %[[arraydecay]] to i32*
+ // CL20: store i32* %[[r1]], i32* addrspace(5)* %lp2, align 4
+ int *lp2 = la;
+
+ // CL12: call void @func1(i32 addrspace(5)* %lv1)
+ // CL20: %[[r2:.*]] = addrspacecast i32 addrspace(5)* %lv1 to i32*
+ // CL20: call void @func1(i32* %[[r2]])
+ func1();
+
+ // CHECK: store i32 4, i32 addrspace(5)* %lvc
+ // CHECK: store i32 4, i32 addrspace(5)* %lv1
+ const int lvc = 4;
+ lv1 = lvc;
+}
Index: test/CodeGenCXX/amdgcn-automatic-variable.cpp
===
--- /dev/null
+++ test/CodeGenCXX/amdgcn-automatic-variable.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -O0 -triple amdgcn---amdgiz -emit-llvm %s -o - | FileCheck %s
+
+// CHECK-LABEL: define void @_Z5func1Pi(i32* %x)
+void func1(int *x) {
+ // CHECK: %[[x_addr:.*]] = alloca i32*{{.*}}addrspace(5)
+ // CHECK: store i32* %x, i32* addrspace(5)* %[[x_addr]]
+ // CHECK: %[[r0:.*]] = load i32*, i32* addrspace(5)* %[[x_addr]]
+ // CHECK: store i32 1, i32* %[[r0]]
+ *x = 1;
+}
+
+// CHECK-LABEL: define void @_Z5func2v()
+void func2(void) {
+ // CHECK: %lv1 = alloca i32, align 4, addrspace(5)
+ // CHECK: %lv2 = alloca i32, align 4, addrspace(5)
+ // CHECK: %la = alloca [100 x i32], align 4, addrspace(5)
+ // CHECK: %lp1 = alloca i32*, align 4, addrspace(5)
+ // CHECK: %lp2 = alloca i32*, align 4, addrspace(5)
+ // CHECK: %lvc = alloca i32, align 4, addrspace(5)
+
+ // CHECK: %[[r0:.*]] = addrspacecast i32 addrspace(5)* %lv1 to i32*
+ // CHECK: store i32 1, i32* %[[r0]]
+ int lv1;
+ lv1 = 1;
+ // CHECK: store i32 2, i32 addrspace(5)* %lv2
+ int lv2 = 2;
+
+ // CHECK: %[[r2:.*]] = addrspacecast [100 x i32] addrspace(5)* %la to [100 x i32]*
+ // CHECK: %[[arrayidx:.*]] = getelementptr inbounds [100 x i32], [100 x i32]* %[[r2]], i64 0, i64 0
+ // CHECK: store i32 3, i32* %[[arrayidx]], align 4
+ int la[100];
+ la[0] = 3;
+
+ // CHECK: store i32* %[[r0]], i32* addrspace(5)* %lp1, align 4
+ int *lp1 =
+
+ // CHECK: %[[arraydecay:.*]]
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:7291 + Address adjustAddrSpaceForAutoVar(Address A, const VarDecl *VD, + CodeGen::CodeGenFunction ) const override; }; yaxunl wrote: > rjmccall wrote: > > yaxunl wrote: > > > rjmccall wrote: > > > > How about, instead of introducing a second method, we just change > > > > performAddrSpaceCast to take two AST address spaces and a flag > > > > indicating whether the address is known to be non-null? Does your > > > > target have an AST-level address space for the stack? > > > In both AST and LLVM, the destination pointee type of address space cast > > > may be different from the source pointee type (e.g. `addrspacecast i32 > > > addrspace(1)* %a to i8 addrspace(4)*` is valid LLVM instruction), so > > > performAddrSpaceCast needs to know the destination QualType, not just the > > > address space. > > Is there any harm to generating separate cast instructions when this > > happens? > They should be equivalent but the LLVM IR will be more verbose and take more > space, and LLVM passes will take more time since previously they only need to > process one instruction but now they need to process two instructions. > > The original representation is more concise, why should we change that to > emit two instructions instead of one instruction? It's a cleaner API. But fine, whatever, feel free to pass down an optional llvm::PointerType* for the destination type. https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:7291 + Address adjustAddrSpaceForAutoVar(Address A, const VarDecl *VD, + CodeGen::CodeGenFunction ) const override; }; rjmccall wrote: > yaxunl wrote: > > rjmccall wrote: > > > How about, instead of introducing a second method, we just change > > > performAddrSpaceCast to take two AST address spaces and a flag indicating > > > whether the address is known to be non-null? Does your target have an > > > AST-level address space for the stack? > > In both AST and LLVM, the destination pointee type of address space cast > > may be different from the source pointee type (e.g. `addrspacecast i32 > > addrspace(1)* %a to i8 addrspace(4)*` is valid LLVM instruction), so > > performAddrSpaceCast needs to know the destination QualType, not just the > > address space. > Is there any harm to generating separate cast instructions when this happens? They should be equivalent but the LLVM IR will be more verbose and take more space, and LLVM passes will take more time since previously they only need to process one instruction but now they need to process two instructions. The original representation is more concise, why should we change that to emit two instructions instead of one instruction? https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:7291 + Address adjustAddrSpaceForAutoVar(Address A, const VarDecl *VD, + CodeGen::CodeGenFunction ) const override; }; yaxunl wrote: > rjmccall wrote: > > How about, instead of introducing a second method, we just change > > performAddrSpaceCast to take two AST address spaces and a flag indicating > > whether the address is known to be non-null? Does your target have an > > AST-level address space for the stack? > In both AST and LLVM, the destination pointee type of address space cast may > be different from the source pointee type (e.g. `addrspacecast i32 > addrspace(1)* %a to i8 addrspace(4)*` is valid LLVM instruction), so > performAddrSpaceCast needs to know the destination QualType, not just the > address space. Is there any harm to generating separate cast instructions when this happens? https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:7291 + Address adjustAddrSpaceForAutoVar(Address A, const VarDecl *VD, + CodeGen::CodeGenFunction ) const override; }; rjmccall wrote: > How about, instead of introducing a second method, we just change > performAddrSpaceCast to take two AST address spaces and a flag indicating > whether the address is known to be non-null? Does your target have an > AST-level address space for the stack? In both AST and LLVM, the destination pointee type of address space cast may be different from the source pointee type (e.g. `addrspacecast i32 addrspace(1)* %a to i8 addrspace(4)*` is valid LLVM instruction), so performAddrSpaceCast needs to know the destination QualType, not just the address space. https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:7291 + Address adjustAddrSpaceForAutoVar(Address A, const VarDecl *VD, + CodeGen::CodeGenFunction ) const override; }; How about, instead of introducing a second method, we just change performAddrSpaceCast to take two AST address spaces and a flag indicating whether the address is known to be non-null? Does your target have an AST-level address space for the stack? https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl updated this revision to Diff 98314.
yaxunl added a comment.
Revised by reviewers' comments.
https://reviews.llvm.org/D32248
Files:
include/clang/AST/Type.h
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CodeGenTypes.cpp
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
test/CodeGen/address-space.c
test/CodeGenCXX/amdgcn-automatic-variable.cpp
test/CodeGenOpenCL/amdgcn-automatic-variable.cl
Index: test/CodeGenOpenCL/amdgcn-automatic-variable.cl
===
--- /dev/null
+++ test/CodeGenOpenCL/amdgcn-automatic-variable.cl
@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 -O0 -cl-std=CL1.2 -triple amdgcn---amdgiz -emit-llvm %s -o - | FileCheck -check-prefixes=CHECK,CL12 %s
+// RUN: %clang_cc1 -O0 -cl-std=CL2.0 -triple amdgcn---amdgiz -emit-llvm %s -o - | FileCheck -check-prefixes=CHECK,CL20 %s
+
+// CL12-LABEL: define void @func1(i32 addrspace(5)* %x)
+// CL20-LABEL: define void @func1(i32* %x)
+void func1(int *x) {
+ // CL12: %[[x_addr:.*]] = alloca i32 addrspace(5)*{{.*}}addrspace(5)
+ // CL12: store i32 addrspace(5)* %x, i32 addrspace(5)* addrspace(5)* %[[x_addr]]
+ // CL12: %[[r0:.*]] = load i32 addrspace(5)*, i32 addrspace(5)* addrspace(5)* %[[x_addr]]
+ // CL12: store i32 1, i32 addrspace(5)* %[[r0]]
+ // CL20: %[[x_addr:.*]] = alloca i32*{{.*}}addrspace(5)
+ // CL20: store i32* %x, i32* addrspace(5)* %[[x_addr]]
+ // CL20: %[[r0:.*]] = load i32*, i32* addrspace(5)* %[[x_addr]]
+ // CL20: store i32 1, i32* %[[r0]]
+ *x = 1;
+}
+
+// CHECK-LABEL: define void @func2()
+void func2(void) {
+ // CHECK: %lv1 = alloca i32, align 4, addrspace(5)
+ // CHECK: %lv2 = alloca i32, align 4, addrspace(5)
+ // CHECK: %la = alloca [100 x i32], align 4, addrspace(5)
+ // CL12: %lp1 = alloca i32 addrspace(5)*, align 4, addrspace(5)
+ // CL12: %lp2 = alloca i32 addrspace(5)*, align 4, addrspace(5)
+ // CL20: %lp1 = alloca i32*, align 4, addrspace(5)
+ // CL20: %lp2 = alloca i32*, align 4, addrspace(5)
+ // CHECK: %lvc = alloca i32, align 4, addrspace(5)
+
+ // CHECK: store i32 1, i32 addrspace(5)* %lv1
+ int lv1;
+ lv1 = 1;
+ // CHECK: store i32 2, i32 addrspace(5)* %lv2
+ int lv2 = 2;
+
+ // CHECK: %[[arrayidx:.*]] = getelementptr inbounds [100 x i32], [100 x i32] addrspace(5)* %la, i64 0, i64 0
+ // CHECK: store i32 3, i32 addrspace(5)* %[[arrayidx]], align 4
+ int la[100];
+ la[0] = 3;
+
+ // CL12: store i32 addrspace(5)* %lv1, i32 addrspace(5)* addrspace(5)* %lp1, align 4
+ // CL20: %[[r0:.*]] = addrspacecast i32 addrspace(5)* %lv1 to i32*
+ // CL20: store i32* %[[r0]], i32* addrspace(5)* %lp1, align 4
+ int *lp1 =
+
+ // CHECK: %[[arraydecay:.*]] = getelementptr inbounds [100 x i32], [100 x i32] addrspace(5)* %la, i32 0, i32 0
+ // CL12: store i32 addrspace(5)* %[[arraydecay]], i32 addrspace(5)* addrspace(5)* %lp2, align 4
+ // CL20: %[[r1:.*]] = addrspacecast i32 addrspace(5)* %[[arraydecay]] to i32*
+ // CL20: store i32* %[[r1]], i32* addrspace(5)* %lp2, align 4
+ int *lp2 = la;
+
+ // CL12: call void @func1(i32 addrspace(5)* %lv1)
+ // CL20: %[[r2:.*]] = addrspacecast i32 addrspace(5)* %lv1 to i32*
+ // CL20: call void @func1(i32* %[[r2]])
+ func1();
+
+ // CHECK: store i32 4, i32 addrspace(5)* %lvc
+ // CHECK: store i32 4, i32 addrspace(5)* %lv1
+ const int lvc = 4;
+ lv1 = lvc;
+}
Index: test/CodeGenCXX/amdgcn-automatic-variable.cpp
===
--- /dev/null
+++ test/CodeGenCXX/amdgcn-automatic-variable.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -O0 -triple amdgcn---amdgiz -emit-llvm %s -o - | FileCheck %s
+
+// CHECK-LABEL: define void @_Z5func1Pi(i32* %x)
+void func1(int *x) {
+ // CHECK: %[[x_addr:.*]] = alloca i32*{{.*}}addrspace(5)
+ // CHECK: store i32* %x, i32* addrspace(5)* %[[x_addr]]
+ // CHECK: %[[r0:.*]] = load i32*, i32* addrspace(5)* %[[x_addr]]
+ // CHECK: store i32 1, i32* %[[r0]]
+ *x = 1;
+}
+
+// CHECK-LABEL: define void @_Z5func2v()
+void func2(void) {
+ // CHECK: %lv1 = alloca i32, align 4, addrspace(5)
+ // CHECK: %lv2 = alloca i32, align 4, addrspace(5)
+ // CHECK: %la = alloca [100 x i32], align 4, addrspace(5)
+ // CHECK: %lp1 = alloca i32*, align 4, addrspace(5)
+ // CHECK: %lp2 = alloca i32*, align 4, addrspace(5)
+ // CHECK: %lvc = alloca i32, align 4, addrspace(5)
+
+ // CHECK: %[[r0:.*]] = addrspacecast i32 addrspace(5)* %lv1 to i32*
+ // CHECK: store i32 1, i32* %[[r0]]
+ int lv1;
+ lv1 = 1;
+ // CHECK: store i32 2, i32 addrspace(5)* %lv2
+ int lv2 = 2;
+
+ // CHECK: %[[r2:.*]] = addrspacecast [100 x i32] addrspace(5)* %la to [100 x i32]*
+ // CHECK: %[[arrayidx:.*]] = getelementptr inbounds [100 x i32], [100 x i32]* %[[r2]], i64 0, i64 0
+ // CHECK: store i32 3, i32* %[[arrayidx]], align 4
+ int la[100];
+ la[0] = 3;
+
+ // CHECK: store i32* %[[r0]], i32* addrspace(5)* %lp1, align 4
+ int *lp1 =
+
+ // CHECK: %[[arraydecay:.*]] = getelementptr inbounds [100 x i32], [100 x i32]* %[[r2]], i32 0, i32
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl marked 2 inline comments as done. yaxunl added inline comments. Comment at: test/CodeGenCXX/amdgcn-automatic-variable.cpp:25 + // CHECK: store i32 2, i32* %[[r1]] + int lv1; + lv1 = 1; yaxunl wrote: > Anastasia wrote: > > I am wondering if all these different test cases are really needed. Are we > > testing any different program paths from the patch? > > > > Also would it make sense to add a test case with an object in an AS > > different to 0 (i.e. with `__attribute__((address_space(n)))`) > I think at least I should cover the typical use cases of auto var. > > I will add a test for __attribute__((address_space(n Sorry. I just checked that C++ does not allow __attribute__((address_space(n))) on automatic var. https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl marked 18 inline comments as done.
yaxunl added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:1115
+ if (AddrTy->getAddressSpace() != ExpectedAddrSpace &&
+ Ty.getAddressSpace() != LangAS::opencl_constant) {
+address = Address(Builder.CreateAddrSpaceCast(Addr,
t-tye wrote:
> Anastasia wrote:
> > Do you need to check this? I thought your change doesn't affect OpenCL
> > because alloca AS == private AS in OpenCL and constant AS local objects
> > are treated as private const objects anyways.
> Given patch D32977 that makes function local variables marked with the
> constant address space treated as static, will they ever be processed by this
> function now? If so then the extra test for opencl_constant would not be
> needed.
Will remove this.
Comment at: test/CodeGenCXX/amdgcn-automatic-variable.cpp:25
+ // CHECK: store i32 2, i32* %[[r1]]
+ int lv1;
+ lv1 = 1;
Anastasia wrote:
> I am wondering if all these different test cases are really needed. Are we
> testing any different program paths from the patch?
>
> Also would it make sense to add a test case with an object in an AS different
> to 0 (i.e. with `__attribute__((address_space(n)))`)
I think at least I should cover the typical use cases of auto var.
I will add a test for __attribute__((address_space(n
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
t-tye added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:1115
+ if (AddrTy->getAddressSpace() != ExpectedAddrSpace &&
+ Ty.getAddressSpace() != LangAS::opencl_constant) {
+address = Address(Builder.CreateAddrSpaceCast(Addr,
Anastasia wrote:
> Do you need to check this? I thought your change doesn't affect OpenCL
> because alloca AS == private AS in OpenCL and constant AS local objects are
> treated as private const objects anyways.
Given patch D32977 that makes function local variables marked with the constant
address space treated as static, will they ever be processed by this function
now? If so then the extra test for opencl_constant would not be needed.
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:1105-1119
+ // Alloca always returns a pointer in alloca address space, which may
+ // be different from the type defined by the language. For example,
+ // in C++ the auto variables are in the default address space. Therefore
+ // cast alloca to the expected address space when necessary.
+ auto Addr = address.getPointer();
+ auto AddrTy = cast(Addr->getType());
+ auto ExpectedAddrSpace =
CGM.getTypes().getVariableType(D)->getAddressSpace();
Anastasia wrote:
> rjmccall wrote:
> > yaxunl wrote:
> > > yaxunl wrote:
> > > > rjmccall wrote:
> > > > > Anastasia wrote:
> > > > > > yaxunl wrote:
> > > > > > > yaxunl wrote:
> > > > > > > > t-tye wrote:
> > > > > > > > > Is any assert done to ensure that it is legal to address
> > > > > > > > > space cast from variable address space to expected address
> > > > > > > > > space? Presumably the language, by definition, will only be
> > > > > > > > > causing legal casts. For example from alloca address space to
> > > > > > > > > generic (which includes the alloca address space).
> > > > > > > > >
> > > > > > > > > For OpenCL, can you explain how the local variable can have
> > > > > > > > > the constant address space and use an alloca for allocation?
> > > > > > > > > Wouldn't a constant address space mean it was static and so
> > > > > > > > > should not be using alloca? And if it is using an alloca, how
> > > > > > > > > can it then be accessed as if it was in constant address
> > > > > > > > > space?
> > > > > > > > If the auto var has address space qualifier specified through
> > > > > > > > `__attribute__((address_space(n)))`, there is not much we can
> > > > > > > > check in clang since it is target dependent. We will just emit
> > > > > > > > address space cast when necessary and let the backend check the
> > > > > > > > validity of the address space cast.
> > > > > > > >
> > > > > > > > Otherwise, for OpenCL, we can assert the expected address space
> > > > > > > > is default (for OpenCL default address space in AST represents
> > > > > > > > private address space in source language) or constant. For
> > > > > > > > other languages we can assert the expected address space
> > > > > > > > qualifier is default (no address space qualifier). It is not
> > > > > > > > convenient to further check whether the emitted LLVM address
> > > > > > > > space cast instruction is valid since it requires target
> > > > > > > > specific information, therefore such check is better deferred
> > > > > > > > to the backend.
> > > > > > > >
> > > > > > > > For OpenCL, currently automatic variable in constant address
> > > > > > > > space is emitted in private address space. For example,
> > > > > > > > currently Clang does not diagnose the following code
> > > > > > > >
> > > > > > > > ```
> > > > > > > > void f(global int* a) {
> > > > > > > > global int* constant p = a;
> > > > > > > > }
> > > > > > > >
> > > > > > > > ```
> > > > > > > > Instead, it emits alloca for p, essentially treats it as
> > > > > > > > `global int* const p`. This seems to be a bug to me (or maybe
> > > > > > > > we can call it a feature? since there seems no better way to
> > > > > > > > translate this to LLVM IR, or simply diagnose this as an
> > > > > > > > error). However, this is better addressed by another patch.
> > > > > > >
> > > > > > > Hi Anastasia,
> > > > > > >
> > > > > > > Any comments about the automatic variable in constant address
> > > > > > > space? Thanks.
> > > > > > From the spec s6.5.3 it feels like we should follow the same
> > > > > > implementation path in Clang for constant AS inside kernel function
> > > > > > as local AS. Because constant AS objects are essentially global
> > > > > > objects.
> > > > > >
> > > > > > Although, we didn't have any issues up to now because const just
> > > > > > does the trick of optimising the objects out eventually. I am not
> > > > > > clear if this creates any issue now with your allocation change. It
> > > > > > feels though that it should probably work fine just as is?
> > > > > If these __constant locals are required to be const (or are
> > > > > implicitly const?) and have constant initializers, it seems to me the
> > > > > implementation obviously intended by the spec is that you allocate
> > > > > them statically in the constant address space. It's likely that just
> > > > > implicitly making the variable static in Sema, the same way you make
> > > > > it implicitly const, will make IRGen and various other parts of the
> > > > > compiler just do the right thing.
> > > > My patch does not change the current behaviour of Clang regarding
> > > > function-scope variable in constant address space. Basically there is
> > > > no issue if there is no address taken. However, if there is address
> > > > taken, there will be assertion due to casting private pointer to
> > > > constant
[PATCH] D32248: CodeGen: Cast alloca to expected address space
Anastasia added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:1105-1119
+ // Alloca always returns a pointer in alloca address space, which may
+ // be different from the type defined by the language. For example,
+ // in C++ the auto variables are in the default address space. Therefore
+ // cast alloca to the expected address space when necessary.
+ auto Addr = address.getPointer();
+ auto AddrTy = cast(Addr->getType());
+ auto ExpectedAddrSpace =
CGM.getTypes().getVariableType(D)->getAddressSpace();
rjmccall wrote:
> yaxunl wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > Anastasia wrote:
> > > > > yaxunl wrote:
> > > > > > yaxunl wrote:
> > > > > > > t-tye wrote:
> > > > > > > > Is any assert done to ensure that it is legal to address space
> > > > > > > > cast from variable address space to expected address space?
> > > > > > > > Presumably the language, by definition, will only be causing
> > > > > > > > legal casts. For example from alloca address space to generic
> > > > > > > > (which includes the alloca address space).
> > > > > > > >
> > > > > > > > For OpenCL, can you explain how the local variable can have the
> > > > > > > > constant address space and use an alloca for allocation?
> > > > > > > > Wouldn't a constant address space mean it was static and so
> > > > > > > > should not be using alloca? And if it is using an alloca, how
> > > > > > > > can it then be accessed as if it was in constant address space?
> > > > > > > If the auto var has address space qualifier specified through
> > > > > > > `__attribute__((address_space(n)))`, there is not much we can
> > > > > > > check in clang since it is target dependent. We will just emit
> > > > > > > address space cast when necessary and let the backend check the
> > > > > > > validity of the address space cast.
> > > > > > >
> > > > > > > Otherwise, for OpenCL, we can assert the expected address space
> > > > > > > is default (for OpenCL default address space in AST represents
> > > > > > > private address space in source language) or constant. For other
> > > > > > > languages we can assert the expected address space qualifier is
> > > > > > > default (no address space qualifier). It is not convenient to
> > > > > > > further check whether the emitted LLVM address space cast
> > > > > > > instruction is valid since it requires target specific
> > > > > > > information, therefore such check is better deferred to the
> > > > > > > backend.
> > > > > > >
> > > > > > > For OpenCL, currently automatic variable in constant address
> > > > > > > space is emitted in private address space. For example, currently
> > > > > > > Clang does not diagnose the following code
> > > > > > >
> > > > > > > ```
> > > > > > > void f(global int* a) {
> > > > > > > global int* constant p = a;
> > > > > > > }
> > > > > > >
> > > > > > > ```
> > > > > > > Instead, it emits alloca for p, essentially treats it as `global
> > > > > > > int* const p`. This seems to be a bug to me (or maybe we can call
> > > > > > > it a feature? since there seems no better way to translate this
> > > > > > > to LLVM IR, or simply diagnose this as an error). However, this
> > > > > > > is better addressed by another patch.
> > > > > >
> > > > > > Hi Anastasia,
> > > > > >
> > > > > > Any comments about the automatic variable in constant address
> > > > > > space? Thanks.
> > > > > From the spec s6.5.3 it feels like we should follow the same
> > > > > implementation path in Clang for constant AS inside kernel function
> > > > > as local AS. Because constant AS objects are essentially global
> > > > > objects.
> > > > >
> > > > > Although, we didn't have any issues up to now because const just
> > > > > does the trick of optimising the objects out eventually. I am not
> > > > > clear if this creates any issue now with your allocation change. It
> > > > > feels though that it should probably work fine just as is?
> > > > If these __constant locals are required to be const (or are implicitly
> > > > const?) and have constant initializers, it seems to me the
> > > > implementation obviously intended by the spec is that you allocate them
> > > > statically in the constant address space. It's likely that just
> > > > implicitly making the variable static in Sema, the same way you make it
> > > > implicitly const, will make IRGen and various other parts of the
> > > > compiler just do the right thing.
> > > My patch does not change the current behaviour of Clang regarding
> > > function-scope variable in constant address space. Basically there is no
> > > issue if there is no address taken. However, if there is address taken,
> > > there will be assertion due to casting private pointer to constant
> > > address space. I think probably it is better to emit function-scope
> > > variable in constant address space as global variable in constant address
> > > space instead of alloca, as
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:1105-1119
+ // Alloca always returns a pointer in alloca address space, which may
+ // be different from the type defined by the language. For example,
+ // in C++ the auto variables are in the default address space. Therefore
+ // cast alloca to the expected address space when necessary.
+ auto Addr = address.getPointer();
+ auto AddrTy = cast(Addr->getType());
+ auto ExpectedAddrSpace =
CGM.getTypes().getVariableType(D)->getAddressSpace();
yaxunl wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > Anastasia wrote:
> > > > yaxunl wrote:
> > > > > yaxunl wrote:
> > > > > > t-tye wrote:
> > > > > > > Is any assert done to ensure that it is legal to address space
> > > > > > > cast from variable address space to expected address space?
> > > > > > > Presumably the language, by definition, will only be causing
> > > > > > > legal casts. For example from alloca address space to generic
> > > > > > > (which includes the alloca address space).
> > > > > > >
> > > > > > > For OpenCL, can you explain how the local variable can have the
> > > > > > > constant address space and use an alloca for allocation? Wouldn't
> > > > > > > a constant address space mean it was static and so should not be
> > > > > > > using alloca? And if it is using an alloca, how can it then be
> > > > > > > accessed as if it was in constant address space?
> > > > > > If the auto var has address space qualifier specified through
> > > > > > `__attribute__((address_space(n)))`, there is not much we can check
> > > > > > in clang since it is target dependent. We will just emit address
> > > > > > space cast when necessary and let the backend check the validity of
> > > > > > the address space cast.
> > > > > >
> > > > > > Otherwise, for OpenCL, we can assert the expected address space is
> > > > > > default (for OpenCL default address space in AST represents private
> > > > > > address space in source language) or constant. For other languages
> > > > > > we can assert the expected address space qualifier is default (no
> > > > > > address space qualifier). It is not convenient to further check
> > > > > > whether the emitted LLVM address space cast instruction is valid
> > > > > > since it requires target specific information, therefore such check
> > > > > > is better deferred to the backend.
> > > > > >
> > > > > > For OpenCL, currently automatic variable in constant address space
> > > > > > is emitted in private address space. For example, currently Clang
> > > > > > does not diagnose the following code
> > > > > >
> > > > > > ```
> > > > > > void f(global int* a) {
> > > > > > global int* constant p = a;
> > > > > > }
> > > > > >
> > > > > > ```
> > > > > > Instead, it emits alloca for p, essentially treats it as `global
> > > > > > int* const p`. This seems to be a bug to me (or maybe we can call
> > > > > > it a feature? since there seems no better way to translate this to
> > > > > > LLVM IR, or simply diagnose this as an error). However, this is
> > > > > > better addressed by another patch.
> > > > >
> > > > > Hi Anastasia,
> > > > >
> > > > > Any comments about the automatic variable in constant address space?
> > > > > Thanks.
> > > > From the spec s6.5.3 it feels like we should follow the same
> > > > implementation path in Clang for constant AS inside kernel function as
> > > > local AS. Because constant AS objects are essentially global objects.
> > > >
> > > > Although, we didn't have any issues up to now because const just does
> > > > the trick of optimising the objects out eventually. I am not clear if
> > > > this creates any issue now with your allocation change. It feels though
> > > > that it should probably work fine just as is?
> > > If these __constant locals are required to be const (or are implicitly
> > > const?) and have constant initializers, it seems to me the implementation
> > > obviously intended by the spec is that you allocate them statically in
> > > the constant address space. It's likely that just implicitly making the
> > > variable static in Sema, the same way you make it implicitly const, will
> > > make IRGen and various other parts of the compiler just do the right
> > > thing.
> > My patch does not change the current behaviour of Clang regarding
> > function-scope variable in constant address space. Basically there is no
> > issue if there is no address taken. However, if there is address taken,
> > there will be assertion due to casting private pointer to constant address
> > space. I think probably it is better to emit function-scope variable in
> > constant address space as global variable in constant address space instead
> > of alloca, as John suggested.
> >
> I agree function-scope variable in constant address space should be emitted
> as global variable in constant address space instead of alloca. However, in
> OpenCL 1.2
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:1105-1119
+ // Alloca always returns a pointer in alloca address space, which may
+ // be different from the type defined by the language. For example,
+ // in C++ the auto variables are in the default address space. Therefore
+ // cast alloca to the expected address space when necessary.
+ auto Addr = address.getPointer();
+ auto AddrTy = cast(Addr->getType());
+ auto ExpectedAddrSpace =
CGM.getTypes().getVariableType(D)->getAddressSpace();
rjmccall wrote:
> Anastasia wrote:
> > yaxunl wrote:
> > > yaxunl wrote:
> > > > t-tye wrote:
> > > > > Is any assert done to ensure that it is legal to address space cast
> > > > > from variable address space to expected address space? Presumably the
> > > > > language, by definition, will only be causing legal casts. For
> > > > > example from alloca address space to generic (which includes the
> > > > > alloca address space).
> > > > >
> > > > > For OpenCL, can you explain how the local variable can have the
> > > > > constant address space and use an alloca for allocation? Wouldn't a
> > > > > constant address space mean it was static and so should not be using
> > > > > alloca? And if it is using an alloca, how can it then be accessed as
> > > > > if it was in constant address space?
> > > > If the auto var has address space qualifier specified through
> > > > `__attribute__((address_space(n)))`, there is not much we can check in
> > > > clang since it is target dependent. We will just emit address space
> > > > cast when necessary and let the backend check the validity of the
> > > > address space cast.
> > > >
> > > > Otherwise, for OpenCL, we can assert the expected address space is
> > > > default (for OpenCL default address space in AST represents private
> > > > address space in source language) or constant. For other languages we
> > > > can assert the expected address space qualifier is default (no address
> > > > space qualifier). It is not convenient to further check whether the
> > > > emitted LLVM address space cast instruction is valid since it requires
> > > > target specific information, therefore such check is better deferred to
> > > > the backend.
> > > >
> > > > For OpenCL, currently automatic variable in constant address space is
> > > > emitted in private address space. For example, currently Clang does not
> > > > diagnose the following code
> > > >
> > > > ```
> > > > void f(global int* a) {
> > > > global int* constant p = a;
> > > > }
> > > >
> > > > ```
> > > > Instead, it emits alloca for p, essentially treats it as `global int*
> > > > const p`. This seems to be a bug to me (or maybe we can call it a
> > > > feature? since there seems no better way to translate this to LLVM IR,
> > > > or simply diagnose this as an error). However, this is better addressed
> > > > by another patch.
> > >
> > > Hi Anastasia,
> > >
> > > Any comments about the automatic variable in constant address space?
> > > Thanks.
> > From the spec s6.5.3 it feels like we should follow the same implementation
> > path in Clang for constant AS inside kernel function as local AS. Because
> > constant AS objects are essentially global objects.
> >
> > Although, we didn't have any issues up to now because const just does the
> > trick of optimising the objects out eventually. I am not clear if this
> > creates any issue now with your allocation change. It feels though that it
> > should probably work fine just as is?
> If these __constant locals are required to be const (or are implicitly
> const?) and have constant initializers, it seems to me the implementation
> obviously intended by the spec is that you allocate them statically in the
> constant address space. It's likely that just implicitly making the variable
> static in Sema, the same way you make it implicitly const, will make IRGen
> and various other parts of the compiler just do the right thing.
My patch does not change the current behaviour of Clang regarding
function-scope variable in constant address space. Basically there is no issue
if there is no address taken. However, if there is address taken, there will be
assertion due to casting private pointer to constant address space. I think
probably it is better to emit function-scope variable in constant address space
as global variable in constant address space instead of alloca, as John
suggested.
Comment at: lib/CodeGen/CGDecl.cpp:1105-1119
+ // Alloca always returns a pointer in alloca address space, which may
+ // be different from the type defined by the language. For example,
+ // in C++ the auto variables are in the default address space. Therefore
+ // cast alloca to the expected address space when necessary.
+ auto Addr = address.getPointer();
+ auto AddrTy = cast(Addr->getType());
+ auto ExpectedAddrSpace =
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:1105-1119
+ // Alloca always returns a pointer in alloca address space, which may
+ // be different from the type defined by the language. For example,
+ // in C++ the auto variables are in the default address space. Therefore
+ // cast alloca to the expected address space when necessary.
+ auto Addr = address.getPointer();
+ auto AddrTy = cast(Addr->getType());
+ auto ExpectedAddrSpace =
CGM.getTypes().getVariableType(D)->getAddressSpace();
Anastasia wrote:
> yaxunl wrote:
> > yaxunl wrote:
> > > t-tye wrote:
> > > > Is any assert done to ensure that it is legal to address space cast
> > > > from variable address space to expected address space? Presumably the
> > > > language, by definition, will only be causing legal casts. For example
> > > > from alloca address space to generic (which includes the alloca address
> > > > space).
> > > >
> > > > For OpenCL, can you explain how the local variable can have the
> > > > constant address space and use an alloca for allocation? Wouldn't a
> > > > constant address space mean it was static and so should not be using
> > > > alloca? And if it is using an alloca, how can it then be accessed as if
> > > > it was in constant address space?
> > > If the auto var has address space qualifier specified through
> > > `__attribute__((address_space(n)))`, there is not much we can check in
> > > clang since it is target dependent. We will just emit address space cast
> > > when necessary and let the backend check the validity of the address
> > > space cast.
> > >
> > > Otherwise, for OpenCL, we can assert the expected address space is
> > > default (for OpenCL default address space in AST represents private
> > > address space in source language) or constant. For other languages we can
> > > assert the expected address space qualifier is default (no address space
> > > qualifier). It is not convenient to further check whether the emitted
> > > LLVM address space cast instruction is valid since it requires target
> > > specific information, therefore such check is better deferred to the
> > > backend.
> > >
> > > For OpenCL, currently automatic variable in constant address space is
> > > emitted in private address space. For example, currently Clang does not
> > > diagnose the following code
> > >
> > > ```
> > > void f(global int* a) {
> > > global int* constant p = a;
> > > }
> > >
> > > ```
> > > Instead, it emits alloca for p, essentially treats it as `global int*
> > > const p`. This seems to be a bug to me (or maybe we can call it a
> > > feature? since there seems no better way to translate this to LLVM IR, or
> > > simply diagnose this as an error). However, this is better addressed by
> > > another patch.
> >
> > Hi Anastasia,
> >
> > Any comments about the automatic variable in constant address space? Thanks.
> From the spec s6.5.3 it feels like we should follow the same implementation
> path in Clang for constant AS inside kernel function as local AS. Because
> constant AS objects are essentially global objects.
>
> Although, we didn't have any issues up to now because const just does the
> trick of optimising the objects out eventually. I am not clear if this
> creates any issue now with your allocation change. It feels though that it
> should probably work fine just as is?
If these __constant locals are required to be const (or are implicitly const?)
and have constant initializers, it seems to me the implementation obviously
intended by the spec is that you allocate them statically in the constant
address space. It's likely that just implicitly making the variable static in
Sema, the same way you make it implicitly const, will make IRGen and various
other parts of the compiler just do the right thing.
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
Anastasia added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:1115
+ if (AddrTy->getAddressSpace() != ExpectedAddrSpace &&
+ Ty.getAddressSpace() != LangAS::opencl_constant) {
+address = Address(Builder.CreateAddrSpaceCast(Addr,
Do you need to check this? I thought your change doesn't affect OpenCL because
alloca AS == private AS in OpenCL and constant AS local objects are treated as
private const objects anyways.
Comment at: test/CodeGen/address-space.c:3
// RUN: %clang_cc1 -triple amdgcn -emit-llvm < %s | FileCheck
-check-prefixes=CHECK,PIZ %s
// RUN: %clang_cc1 -triple amdgcn---amdgiz -emit-llvm < %s | FileCheck
-check-prefixes=CHeCK,GIZ %s
CHeCK -> CHECK
Comment at: test/CodeGenCXX/amdgcn-automatic-variable.cpp:25
+ // CHECK: store i32 2, i32* %[[r1]]
+ int lv1;
+ lv1 = 1;
I am wondering if all these different test cases are really needed. Are we
testing any different program paths from the patch?
Also would it make sense to add a test case with an object in an AS different
to 0 (i.e. with `__attribute__((address_space(n)))`)
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
Anastasia added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:1105-1119
+ // Alloca always returns a pointer in alloca address space, which may
+ // be different from the type defined by the language. For example,
+ // in C++ the auto variables are in the default address space. Therefore
+ // cast alloca to the expected address space when necessary.
+ auto Addr = address.getPointer();
+ auto AddrTy = cast(Addr->getType());
+ auto ExpectedAddrSpace =
CGM.getTypes().getVariableType(D)->getAddressSpace();
yaxunl wrote:
> yaxunl wrote:
> > t-tye wrote:
> > > Is any assert done to ensure that it is legal to address space cast from
> > > variable address space to expected address space? Presumably the
> > > language, by definition, will only be causing legal casts. For example
> > > from alloca address space to generic (which includes the alloca address
> > > space).
> > >
> > > For OpenCL, can you explain how the local variable can have the constant
> > > address space and use an alloca for allocation? Wouldn't a constant
> > > address space mean it was static and so should not be using alloca? And
> > > if it is using an alloca, how can it then be accessed as if it was in
> > > constant address space?
> > If the auto var has address space qualifier specified through
> > `__attribute__((address_space(n)))`, there is not much we can check in
> > clang since it is target dependent. We will just emit address space cast
> > when necessary and let the backend check the validity of the address space
> > cast.
> >
> > Otherwise, for OpenCL, we can assert the expected address space is default
> > (for OpenCL default address space in AST represents private address space
> > in source language) or constant. For other languages we can assert the
> > expected address space qualifier is default (no address space qualifier).
> > It is not convenient to further check whether the emitted LLVM address
> > space cast instruction is valid since it requires target specific
> > information, therefore such check is better deferred to the backend.
> >
> > For OpenCL, currently automatic variable in constant address space is
> > emitted in private address space. For example, currently Clang does not
> > diagnose the following code
> >
> > ```
> > void f(global int* a) {
> > global int* constant p = a;
> > }
> >
> > ```
> > Instead, it emits alloca for p, essentially treats it as `global int* const
> > p`. This seems to be a bug to me (or maybe we can call it a feature? since
> > there seems no better way to translate this to LLVM IR, or simply diagnose
> > this as an error). However, this is better addressed by another patch.
>
> Hi Anastasia,
>
> Any comments about the automatic variable in constant address space? Thanks.
From the spec s6.5.3 it feels like we should follow the same implementation
path in Clang for constant AS inside kernel function as local AS. Because
constant AS objects are essentially global objects.
Although, we didn't have any issues up to now because const just does the
trick of optimising the objects out eventually. I am not clear if this creates
any issue now with your allocation change. It feels though that it should
probably work fine just as is?
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:1105-1119
+ // Alloca always returns a pointer in alloca address space, which may
+ // be different from the type defined by the language. For example,
+ // in C++ the auto variables are in the default address space. Therefore
+ // cast alloca to the expected address space when necessary.
+ auto Addr = address.getPointer();
+ auto AddrTy = cast(Addr->getType());
+ auto ExpectedAddrSpace =
CGM.getTypes().getVariableType(D)->getAddressSpace();
yaxunl wrote:
> t-tye wrote:
> > Is any assert done to ensure that it is legal to address space cast from
> > variable address space to expected address space? Presumably the language,
> > by definition, will only be causing legal casts. For example from alloca
> > address space to generic (which includes the alloca address space).
> >
> > For OpenCL, can you explain how the local variable can have the constant
> > address space and use an alloca for allocation? Wouldn't a constant address
> > space mean it was static and so should not be using alloca? And if it is
> > using an alloca, how can it then be accessed as if it was in constant
> > address space?
> If the auto var has address space qualifier specified through
> `__attribute__((address_space(n)))`, there is not much we can check in clang
> since it is target dependent. We will just emit address space cast when
> necessary and let the backend check the validity of the address space cast.
>
> Otherwise, for OpenCL, we can assert the expected address space is default
> (for OpenCL default address space in AST represents private address space in
> source language) or constant. For other languages we can assert the expected
> address space qualifier is default (no address space qualifier). It is not
> convenient to further check whether the emitted LLVM address space cast
> instruction is valid since it requires target specific information, therefore
> such check is better deferred to the backend.
>
> For OpenCL, currently automatic variable in constant address space is emitted
> in private address space. For example, currently Clang does not diagnose the
> following code
>
> ```
> void f(global int* a) {
> global int* constant p = a;
> }
>
> ```
> Instead, it emits alloca for p, essentially treats it as `global int* const
> p`. This seems to be a bug to me (or maybe we can call it a feature? since
> there seems no better way to translate this to LLVM IR, or simply diagnose
> this as an error). However, this is better addressed by another patch.
Hi Anastasia,
Any comments about the automatic variable in constant address space? Thanks.
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1118 +AddrTy->getElementType()->getPointerTo(ExpectedAddrSpace)), +address.getAlignment()); + } This is a lot of work to be doing in a pretty common routine for the benefit of one unusual target. Maybe there's some way to fast-path it, like you can cache the AST address space of the stack on the CGM (assuming it's always got its own AST address space, I guess) and just check whether Ty.getAddressSpace() is different from that. Also, I feel like general routines in IRGen should never be calling CreateAddrSpaceCast; they should always be calling getTargetCodeGenInfo().performAddrSpaceCast instead, just in case the target has some more specific idea about how the conversion should be done. I know we're not consistent about that right now, but it's something to aim for. Also, I agree with Tony Tye: if it's an intended feature that you can declare locals in the constant address space, you need to figure out what the allocation semantics are for that and emit the appropriate code rather than just hand-waving it here after the fact. Comment at: lib/CodeGen/CodeGenTypes.h:200 + /// Get the LLVM pointer type of a variable. + llvm::PointerType *getVariableType(VarDecl D); + yaxunl wrote: > t-tye wrote: > > Should the name reflect that the type returned is not the variable type, > > but a pointer to the variable type? For example, getVariablePointerType or > > getPointerToVariableType. > I think getPointerToVariableType is better. I will change it. The only time you use this function is to immediately call getAddressSpace on it. Seems to me you should just call getTargetAddressSpace(D.getType()). https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:1105-1119
+ // Alloca always returns a pointer in alloca address space, which may
+ // be different from the type defined by the language. For example,
+ // in C++ the auto variables are in the default address space. Therefore
+ // cast alloca to the expected address space when necessary.
+ auto Addr = address.getPointer();
+ auto AddrTy = cast(Addr->getType());
+ auto ExpectedAddrSpace =
CGM.getTypes().getVariableType(D)->getAddressSpace();
t-tye wrote:
> Is any assert done to ensure that it is legal to address space cast from
> variable address space to expected address space? Presumably the language, by
> definition, will only be causing legal casts. For example from alloca address
> space to generic (which includes the alloca address space).
>
> For OpenCL, can you explain how the local variable can have the constant
> address space and use an alloca for allocation? Wouldn't a constant address
> space mean it was static and so should not be using alloca? And if it is
> using an alloca, how can it then be accessed as if it was in constant address
> space?
If the auto var has address space qualifier specified through
`__attribute__((address_space(n)))`, there is not much we can check in clang
since it is target dependent. We will just emit address space cast when
necessary and let the backend check the validity of the address space cast.
Otherwise, for OpenCL, we can assert the expected address space is default (for
OpenCL default address space in AST represents private address space in source
language) or constant. For other languages we can assert the expected address
space qualifier is default (no address space qualifier). It is not convenient
to further check whether the emitted LLVM address space cast instruction is
valid since it requires target specific information, therefore such check is
better deferred to the backend.
For OpenCL, currently automatic variable in constant address space is emitted
in private address space. For example, currently Clang does not diagnose the
following code
```
void f(global int* a) {
global int* constant p = a;
}
```
Instead, it emits alloca for p, essentially treats it as `global int* const p`.
This seems to be a bug to me (or maybe we can call it a feature? since there
seems no better way to translate this to LLVM IR, or simply diagnose this as an
error). However, this is better addressed by another patch.
Comment at: lib/CodeGen/CodeGenTypes.h:200
+ /// Get the LLVM pointer type of a variable.
+ llvm::PointerType *getVariableType(VarDecl D);
+
t-tye wrote:
> Should the name reflect that the type returned is not the variable type, but
> a pointer to the variable type? For example, getVariablePointerType or
> getPointerToVariableType.
I think getPointerToVariableType is better. I will change it.
https://reviews.llvm.org/D32248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
t-tye added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1105-1119 + // Alloca always returns a pointer in alloca address space, which may + // be different from the type defined by the language. For example, + // in C++ the auto variables are in the default address space. Therefore + // cast alloca to the expected address space when necessary. + auto Addr = address.getPointer(); + auto AddrTy = cast(Addr->getType()); + auto ExpectedAddrSpace = CGM.getTypes().getVariableType(D)->getAddressSpace(); Is any assert done to ensure that it is legal to address space cast from variable address space to expected address space? Presumably the language, by definition, will only be causing legal casts. For example from alloca address space to generic (which includes the alloca address space). For OpenCL, can you explain how the local variable can have the constant address space and use an alloca for allocation? Wouldn't a constant address space mean it was static and so should not be using alloca? And if it is using an alloca, how can it then be accessed as if it was in constant address space? Comment at: lib/CodeGen/CodeGenTypes.h:200 + /// Get the LLVM pointer type of a variable. + llvm::PointerType *getVariableType(VarDecl D); + Should the name reflect that the type returned is not the variable type, but a pointer to the variable type? For example, getVariablePointerType or getPointerToVariableType. https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32248: CodeGen: Cast alloca to expected address space
yaxunl created this revision.
Alloca always returns a pointer in alloca address space, which may
be different from the type defined by the language. For example,
in C++ the auto variables are in the default address space. Therefore
cast alloca to the expected address space when necessary.
https://reviews.llvm.org/D32248
Files:
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CodeGenTypes.cpp
lib/CodeGen/CodeGenTypes.h
test/CodeGen/address-space.c
test/CodeGenCXX/amdgcn-automatic-variable.cpp
Index: test/CodeGenCXX/amdgcn-automatic-variable.cpp
===
--- /dev/null
+++ test/CodeGenCXX/amdgcn-automatic-variable.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -O0 -triple amdgcn---amdgiz -emit-llvm %s -o - | FileCheck %s
+
+// CHECK-LABEL: define void @_Z5func1Pi(i32* %x)
+void func1(int *x) {
+ // CHECK: %[[x_addr:.*]] = alloca i32*{{.*}}addrspace(5)
+ // CHECK: store i32* %x, i32* addrspace(5)* %[[x_addr]]
+ // CHECK: %[[r0:.*]] = load i32*, i32* addrspace(5)* %[[x_addr]]
+ // CHECK: store i32 1, i32* %[[r0]]
+ *x = 1;
+}
+
+// CHECK-LABEL: define void @_Z5func2v()
+void func2(void) {
+ // CHECK: %lv1 = alloca i32, align 4, addrspace(5)
+ // CHECK: %lv2 = alloca i32, align 4, addrspace(5)
+ // CHECK: %la = alloca [100 x i32], align 4, addrspace(5)
+ // CHECK: %lp1 = alloca i32*, align 4, addrspace(5)
+ // CHECK: %lp2 = alloca i32*, align 4, addrspace(5)
+ // CHECK: %lvc = alloca i32, align 4, addrspace(5)
+
+ // CHECK: %[[r0:.*]] = addrspacecast i32 addrspace(5)* %lv1 to i32*
+ // CHECK: store i32 1, i32* %[[r0]]
+ // CHECK: %[[r1:.*]] = addrspacecast i32 addrspace(5)* %lv2 to i32*
+ // CHECK: store i32 2, i32* %[[r1]]
+ int lv1;
+ lv1 = 1;
+ int lv2 = 2;
+
+ // CHECK: %[[r2:.*]] = addrspacecast [100 x i32] addrspace(5)* %la to [100 x i32]*
+ // CHECK: %[[arrayidx:.*]] = getelementptr inbounds [100 x i32], [100 x i32]* %[[r2]], i64 0, i64 0
+ // CHECK: store i32 3, i32* %[[arrayidx]], align 4
+ int la[100];
+ la[0] = 3;
+
+ // CHECK: %[[r3:.*]] = addrspacecast i32* addrspace(5)* %lp1 to i32**
+ // CHECK: store i32* %[[r0]], i32** %[[r3]], align 4
+ int *lp1 =
+
+ // CHECK: %[[r4:.*]] = addrspacecast i32* addrspace(5)* %lp2 to i32**
+ // CHECK: %[[arraydecay:.*]] = getelementptr inbounds [100 x i32], [100 x i32]* %[[r2]], i32 0, i32 0
+ // CHECK: store i32* %[[arraydecay]], i32** %[[r4]], align 4
+ int *lp2 = la;
+
+ // CHECK: call void @_Z5func1Pi(i32* %[[r0]])
+ func1();
+
+ // CHECK: %[[r5:.*]] = addrspacecast i32 addrspace(5)* %lvc to i32*
+ // CHECK: store i32 4, i32* %[[r5]]
+ // CHECK: store i32 4, i32* %[[r0]]
+ const int lvc = 4;
+ lv1 = lvc;
+}
Index: test/CodeGen/address-space.c
===
--- test/CodeGen/address-space.c
+++ test/CodeGen/address-space.c
@@ -40,8 +40,10 @@
} MyStruct;
// CHECK-LABEL: define void @test4(
-// CHECK: call void @llvm.memcpy.p0i8.p2i8
-// CHECK: call void @llvm.memcpy.p2i8.p0i8
+// GIZ: call void @llvm.memcpy.p0i8.p2i8
+// GIZ: call void @llvm.memcpy.p2i8.p0i8
+// PIZ: call void @llvm.memcpy.p4i8.p2i8
+// PIZ: call void @llvm.memcpy.p2i8.p4i8
void test4(MyStruct __attribute__((address_space(2))) *pPtr) {
MyStruct s = pPtr[0];
pPtr[0] = s;
Index: lib/CodeGen/CodeGenTypes.h
===
--- lib/CodeGen/CodeGenTypes.h
+++ lib/CodeGen/CodeGenTypes.h
@@ -196,6 +196,9 @@
/// memory representation is usually i8 or i32, depending on the target.
llvm::Type *ConvertTypeForMem(QualType T);
+ /// Get the LLVM pointer type of a variable.
+ llvm::PointerType *getVariableType(VarDecl D);
+
/// GetFunctionType - Get the LLVM function type for \arg Info.
llvm::FunctionType *GetFunctionType(const CGFunctionInfo );
Index: lib/CodeGen/CodeGenTypes.cpp
===
--- lib/CodeGen/CodeGenTypes.cpp
+++ lib/CodeGen/CodeGenTypes.cpp
@@ -92,6 +92,11 @@
(unsigned)Context.getTypeSize(T));
}
+llvm::PointerType *CodeGenTypes::getVariableType(VarDecl D) {
+ auto Ty = D.getType();
+ return ConvertTypeForMem(Ty)->getPointerTo(
+ getContext().getTargetAddressSpace(Ty));
+}
/// isRecordLayoutComplete - Return true if the specified type is already
/// completely laid out.
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1102,6 +1102,22 @@
address = Address(vla, alignment);
}
+ // Alloca always returns a pointer in alloca address space, which may
+ // be different from the type defined by the language. For example,
+ // in C++ the auto variables are in the default address space. Therefore
+ // cast alloca to the expected address space when necessary.
+ auto Addr = address.getPointer();
+ auto AddrTy = cast(Addr->getType());
+ auto ExpectedAddrSpace =
