[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
rjmccall added inline comments.
Comment at: cfe/trunk/lib/CodeGen/CGExpr.cpp:4268
+DestTy.getAddressSpace(), ConvertType(DestTy));
+return MakeNaturalAlignPointeeAddrLValue(V, DestTy);
+ }
romanovvlad wrote:
> Hi,
>
> It seems this code doesn't work correctly(repro at the end). TBAA information
> is lost here because MakeNaturalAlignPointeeAddrLValue constructs LValue with
> alignment of poinee type but TBAA info is taken from pointer itself what is
> strange enough. As a result, for example, memcpy with wrong size is generated
> for copy constructors.
>
> Repro:
>
> ```
> class P {
> public:
> P(const P ) = default;
>
> long a;
> long b;
> };
>
> __kernel void foo(__global P* GPtr) {
> P Val = GPtr[0];
> }
> ```
>
> As a solution the line could be replaced with the following:
> ```
> return MakeAddrLValue(Address(V, LV.getAddress().getAlignment()),
>E->getType(), LV.getBaseInfo(),
>CGM.getTBAAInfoForSubobject(LV,
> E->getType()));
> ```
> To take all the information from the original pointer.
>
> What do you think about solution?
>
Oh, yes, this should absolutely not be using
`MakeNaturalAlignPointerAddrLValue`; it should be preserving all of the extra
information from the original l-value, as you say.
I think TBAA information is independent of address-space qualification and can
just be taken from the original LV directly instead of using
`getTBAAInfoForSubobject`.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53764/new/
https://reviews.llvm.org/D53764
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
romanovvlad added inline comments.
Comment at: cfe/trunk/lib/CodeGen/CGExpr.cpp:4268
+DestTy.getAddressSpace(), ConvertType(DestTy));
+return MakeNaturalAlignPointeeAddrLValue(V, DestTy);
+ }
Hi,
It seems this code doesn't work correctly(repro at the end). TBAA information
is lost here because MakeNaturalAlignPointeeAddrLValue constructs LValue with
alignment of poinee type but TBAA info is taken from pointer itself what is
strange enough. As a result, for example, memcpy with wrong size is generated
for copy constructors.
Repro:
```
class P {
public:
P(const P ) = default;
long a;
long b;
};
__kernel void foo(__global P* GPtr) {
P Val = GPtr[0];
}
```
As a solution the line could be replaced with the following:
```
return MakeAddrLValue(Address(V, LV.getAddress().getAlignment()),
E->getType(), LV.getBaseInfo(),
CGM.getTBAAInfoForSubobject(LV,
E->getType()));
```
To take all the information from the original pointer.
What do you think about solution?
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53764/new/
https://reviews.llvm.org/D53764
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
This revision was automatically updated to reflect the committed changes.
Closed by commit rL347059: [OpenCL] Enable address spaces for references in C++
(authored by stulova, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D53764?vs=174033=174378#toc
Repository:
rL LLVM
https://reviews.llvm.org/D53764
Files:
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/AST/Expr.cpp
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/lib/Sema/DeclSpec.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaExprCXX.cpp
cfe/trunk/lib/Sema/SemaInit.cpp
cfe/trunk/lib/Sema/SemaType.cpp
cfe/trunk/test/CodeGenOpenCLCXX/address-space-deduction.cl
Index: cfe/trunk/lib/CodeGen/CGExpr.cpp
===
--- cfe/trunk/lib/CodeGen/CGExpr.cpp
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp
@@ -4163,7 +4163,6 @@
case CK_ARCReclaimReturnedObject:
case CK_ARCExtendBlockObject:
case CK_CopyAndAutoreleaseBlockObject:
- case CK_AddressSpaceConversion:
case CK_IntToOCLSampler:
case CK_FixedPointCast:
case CK_FixedPointToBoolean:
@@ -4260,6 +4259,14 @@
return MakeAddrLValue(V, E->getType(), LV.getBaseInfo(),
CGM.getTBAAInfoForSubobject(LV, E->getType()));
}
+ case CK_AddressSpaceConversion: {
+LValue LV = EmitLValue(E->getSubExpr());
+QualType DestTy = getContext().getPointerType(E->getType());
+llvm::Value *V = getTargetHooks().performAddrSpaceCast(
+*this, LV.getPointer(), E->getSubExpr()->getType().getAddressSpace(),
+DestTy.getAddressSpace(), ConvertType(DestTy));
+return MakeNaturalAlignPointeeAddrLValue(V, DestTy);
+ }
case CK_ObjCObjectLValueCast: {
LValue LV = EmitLValue(E->getSubExpr());
Address V = Builder.CreateElementBitCast(LV.getAddress(),
Index: cfe/trunk/lib/AST/Expr.cpp
===
--- cfe/trunk/lib/AST/Expr.cpp
+++ cfe/trunk/lib/AST/Expr.cpp
@@ -1634,13 +1634,18 @@
assert(getSubExpr()->getType()->isFunctionType());
goto CheckNoBasePath;
- case CK_AddressSpaceConversion:
-assert(getType()->isPointerType() || getType()->isBlockPointerType());
-assert(getSubExpr()->getType()->isPointerType() ||
- getSubExpr()->getType()->isBlockPointerType());
-assert(getType()->getPointeeType().getAddressSpace() !=
- getSubExpr()->getType()->getPointeeType().getAddressSpace());
-LLVM_FALLTHROUGH;
+ case CK_AddressSpaceConversion: {
+auto Ty = getType();
+auto SETy = getSubExpr()->getType();
+assert(getValueKindForType(Ty) == Expr::getValueKindForType(SETy));
+if (!isGLValue())
+ Ty = Ty->getPointeeType();
+if (!isGLValue())
+ SETy = SETy->getPointeeType();
+assert(!Ty.isNull() && !SETy.isNull() &&
+ Ty.getAddressSpace() != SETy.getAddressSpace());
+goto CheckNoBasePath;
+ }
// These should not have an inheritance path.
case CK_Dynamic:
case CK_ToUnion:
Index: cfe/trunk/lib/Sema/SemaType.cpp
===
--- cfe/trunk/lib/Sema/SemaType.cpp
+++ cfe/trunk/lib/Sema/SemaType.cpp
@@ -7181,7 +7181,8 @@
bool IsPointee =
ChunkIndex > 0 &&
(D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Pointer ||
- D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer);
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer ||
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Reference);
bool IsFuncReturnType =
ChunkIndex > 0 &&
D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Function;
Index: cfe/trunk/lib/Sema/DeclSpec.cpp
===
--- cfe/trunk/lib/Sema/DeclSpec.cpp
+++ cfe/trunk/lib/Sema/DeclSpec.cpp
@@ -566,14 +566,16 @@
// these storage-class specifiers.
// OpenCL v1.2 s6.8 changes this to "The auto and register storage-class
// specifiers are not supported."
+ // OpenCL C++ v1.0 s2.9 restricts register.
if (S.getLangOpts().OpenCL &&
!S.getOpenCLOptions().isEnabled("cl_clang_storage_class_specifiers")) {
switch (SC) {
case SCS_extern:
case SCS_private_extern:
case SCS_static:
- if (S.getLangOpts().OpenCLVersion < 120) {
-DiagID = diag::err_opencl_unknown_type_specifier;
+ if (S.getLangOpts().OpenCLVersion < 120 &&
+ !S.getLangOpts().OpenCLCPlusPlus) {
+DiagID = diag::err_opencl_unknown_type_specifier;
PrevSpec = getSpecifierName(SC);
return true;
}
Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -7352,19 +7352,23 @@
return;
}
}
-// OpenCL v1.2 s6.5 - All program scope variables must be declared in the
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. https://reviews.llvm.org/D53764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
Anastasia marked 4 inline comments as done.
Anastasia added a comment.
Do you think there is anything else to do for this patch?
Comment at: lib/Sema/SemaExprCXX.cpp:4289
+ /*BasePath=*/nullptr, CCK)
+ .get();
rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Anastasia wrote:
> > > > rjmccall wrote:
> > > > > Anastasia wrote:
> > > > > > rjmccall wrote:
> > > > > > > Okay. But if `ToType` *isn't* a reference type, this will never
> > > > > > > be an address-space conversion. I feel like this code could be
> > > > > > > written more clearly to express what it's trying to do.
> > > > > > I hope it makes more sense now. Btw, it also applies to pointer
> > > > > > type.
> > > > > The logic is wrong for pointer types; if you're converting pointers,
> > > > > you need to be checking the address space of the pointee type of the
> > > > > from type.
> > > > >
> > > > > It sounds like this is totally inadequately tested; please flesh out
> > > > > the test with all of these cases. While you're at it, please ensure
> > > > > that there are tests verifying that we don't allowing address-space
> > > > > changes in nested positions.
> > > > Thanks for spotting this bug! The generated IR for the test was still
> > > > correct because AS of `FromType` happened to correctly mismatch AS of
> > > > pointee of `ToType`.
> > > >
> > > > I failed to construct the test case where it would miss classifying
> > > > `addrspacecast` due to OpenCL or C++ sema rules but I managed to add a
> > > > case in which `addrspacecast` was incorrectly added for pointers where
> > > > it wasn't needed (see line 36 of the test). I think this code is
> > > > covered now.
> > > >
> > > > As for the address space position in pointers, the following test
> > > > checks the address spaces of pointers in `addrspacecast`. For the other
> > > > program paths we also have a test with similar checks in
> > > > `test/CodeGenOpenCL/address-spaces-conversions.cl` that we now run for
> > > > C++ mode too.
> > > >
> > > > BTW, while trying to construct a test case for the bug, I have
> > > > discovered that multiple pointer indirection casting isn't working
> > > > correctly. I.e. for the following program:
> > > > kernel void foo(){
> > > > __private int** loc;
> > > > int** loc_p = loc;
> > > > **loc_p = 1;
> > > > }
> > > > We generate:
> > > > bitcast i32* addrspace(4)* %0 to i32 addrspace(4)* addrspace(4)*
> > > > in OpenCL C and then perform `store` over pointer in AS 4 (generic). We
> > > > have now lost the information that the original pointer was in
> > > > `private` AS and that the adjustment of AS segment has to be performed
> > > > before accessing memory pointed by the pointer. Based on the current
> > > > specification of `addrspacecast` in
> > > > https://llvm.org/docs/LangRef.html#addrspacecast-to-instruction I am
> > > > not very clear whether it can be used for this case without any
> > > > modifications or clarifications and also what would happen if there are
> > > > multiple AS mismatches. I am going to look at this issue separately in
> > > > more details. In OpenCL C++ an ICE is triggered for this though. Let me
> > > > know if you have any thoughts on this.
> > > Thanks, the check looks good now.
> > >
> > > > BTW, while trying to construct a test case for the bug, I have
> > > > discovered that multiple pointer indirection casting isn't working
> > > > correctly.
> > >
> > > This needs to be an error in Sema. The only qualification conversions
> > > that should be allowed in general on nested pointers (i.e. on `T` in
> > > `T**` or `T*&`) are the basic C qualifiers: `const`, `volatile`, and
> > > `restrict`; any other qualification change there is unsound.
> > I see. I guess it's because C++ rules don't cover address spaces.
> >
> > It feels like it would be a regression for OpenCL C++ vs OpenCL C to reject
> > nested pointers with address spaces because it was allowed before. :(
> >
> > However, the generation for OpenCL C and C are incorrect currently. I will
> > try to sort that all out as a separate patch though, if it makes sense?
> C++'s rules assume that qualifiers don't introduce real representation
> differences and that operations on qualified types are compatible with
> operations on unqualified types. That's not true of qualifiers in general:
> address space qualifiers can change representations, ARC qualifiers can have
> incompatible semantics, etc. There is no way to soundly implement a
> conversion from `__private int **` to `__generic int **`, just there's no way
> to soundly implement a conversion from `Derived **` to `Base **`.
>
> If you want to allow this conversion anyway for source-compatibility reasons
> (and I don't think that's a good idea), it should be a bitcast.
Ok, then `bitcast` is not a good solution because it has an issue of
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
rjmccall added inline comments.
Comment at: lib/Sema/SemaExprCXX.cpp:4289
+ /*BasePath=*/nullptr, CCK)
+ .get();
Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > Anastasia wrote:
> > > > > rjmccall wrote:
> > > > > > Okay. But if `ToType` *isn't* a reference type, this will never be
> > > > > > an address-space conversion. I feel like this code could be
> > > > > > written more clearly to express what it's trying to do.
> > > > > I hope it makes more sense now. Btw, it also applies to pointer type.
> > > > The logic is wrong for pointer types; if you're converting pointers,
> > > > you need to be checking the address space of the pointee type of the
> > > > from type.
> > > >
> > > > It sounds like this is totally inadequately tested; please flesh out
> > > > the test with all of these cases. While you're at it, please ensure
> > > > that there are tests verifying that we don't allowing address-space
> > > > changes in nested positions.
> > > Thanks for spotting this bug! The generated IR for the test was still
> > > correct because AS of `FromType` happened to correctly mismatch AS of
> > > pointee of `ToType`.
> > >
> > > I failed to construct the test case where it would miss classifying
> > > `addrspacecast` due to OpenCL or C++ sema rules but I managed to add a
> > > case in which `addrspacecast` was incorrectly added for pointers where it
> > > wasn't needed (see line 36 of the test). I think this code is covered now.
> > >
> > > As for the address space position in pointers, the following test checks
> > > the address spaces of pointers in `addrspacecast`. For the other program
> > > paths we also have a test with similar checks in
> > > `test/CodeGenOpenCL/address-spaces-conversions.cl` that we now run for
> > > C++ mode too.
> > >
> > > BTW, while trying to construct a test case for the bug, I have discovered
> > > that multiple pointer indirection casting isn't working correctly. I.e.
> > > for the following program:
> > > kernel void foo(){
> > > __private int** loc;
> > > int** loc_p = loc;
> > > **loc_p = 1;
> > > }
> > > We generate:
> > > bitcast i32* addrspace(4)* %0 to i32 addrspace(4)* addrspace(4)*
> > > in OpenCL C and then perform `store` over pointer in AS 4 (generic). We
> > > have now lost the information that the original pointer was in `private`
> > > AS and that the adjustment of AS segment has to be performed before
> > > accessing memory pointed by the pointer. Based on the current
> > > specification of `addrspacecast` in
> > > https://llvm.org/docs/LangRef.html#addrspacecast-to-instruction I am not
> > > very clear whether it can be used for this case without any modifications
> > > or clarifications and also what would happen if there are multiple AS
> > > mismatches. I am going to look at this issue separately in more details.
> > > In OpenCL C++ an ICE is triggered for this though. Let me know if you
> > > have any thoughts on this.
> > Thanks, the check looks good now.
> >
> > > BTW, while trying to construct a test case for the bug, I have discovered
> > > that multiple pointer indirection casting isn't working correctly.
> >
> > This needs to be an error in Sema. The only qualification conversions that
> > should be allowed in general on nested pointers (i.e. on `T` in `T**` or
> > `T*&`) are the basic C qualifiers: `const`, `volatile`, and `restrict`; any
> > other qualification change there is unsound.
> I see. I guess it's because C++ rules don't cover address spaces.
>
> It feels like it would be a regression for OpenCL C++ vs OpenCL C to reject
> nested pointers with address spaces because it was allowed before. :(
>
> However, the generation for OpenCL C and C are incorrect currently. I will
> try to sort that all out as a separate patch though, if it makes sense?
C++'s rules assume that qualifiers don't introduce real representation
differences and that operations on qualified types are compatible with
operations on unqualified types. That's not true of qualifiers in general:
address space qualifiers can change representations, ARC qualifiers can have
incompatible semantics, etc. There is no way to soundly implement a conversion
from `__private int **` to `__generic int **`, just there's no way to soundly
implement a conversion from `Derived **` to `Base **`.
If you want to allow this conversion anyway for source-compatibility reasons
(and I don't think that's a good idea), it should be a bitcast.
https://reviews.llvm.org/D53764
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
Anastasia added inline comments.
Comment at: lib/Sema/SemaExprCXX.cpp:4289
+ /*BasePath=*/nullptr, CCK)
+ .get();
rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Anastasia wrote:
> > > > rjmccall wrote:
> > > > > Okay. But if `ToType` *isn't* a reference type, this will never be
> > > > > an address-space conversion. I feel like this code could be written
> > > > > more clearly to express what it's trying to do.
> > > > I hope it makes more sense now. Btw, it also applies to pointer type.
> > > The logic is wrong for pointer types; if you're converting pointers, you
> > > need to be checking the address space of the pointee type of the from
> > > type.
> > >
> > > It sounds like this is totally inadequately tested; please flesh out the
> > > test with all of these cases. While you're at it, please ensure that
> > > there are tests verifying that we don't allowing address-space changes in
> > > nested positions.
> > Thanks for spotting this bug! The generated IR for the test was still
> > correct because AS of `FromType` happened to correctly mismatch AS of
> > pointee of `ToType`.
> >
> > I failed to construct the test case where it would miss classifying
> > `addrspacecast` due to OpenCL or C++ sema rules but I managed to add a case
> > in which `addrspacecast` was incorrectly added for pointers where it wasn't
> > needed (see line 36 of the test). I think this code is covered now.
> >
> > As for the address space position in pointers, the following test checks
> > the address spaces of pointers in `addrspacecast`. For the other program
> > paths we also have a test with similar checks in
> > `test/CodeGenOpenCL/address-spaces-conversions.cl` that we now run for C++
> > mode too.
> >
> > BTW, while trying to construct a test case for the bug, I have discovered
> > that multiple pointer indirection casting isn't working correctly. I.e. for
> > the following program:
> > kernel void foo(){
> > __private int** loc;
> > int** loc_p = loc;
> > **loc_p = 1;
> > }
> > We generate:
> > bitcast i32* addrspace(4)* %0 to i32 addrspace(4)* addrspace(4)*
> > in OpenCL C and then perform `store` over pointer in AS 4 (generic). We
> > have now lost the information that the original pointer was in `private` AS
> > and that the adjustment of AS segment has to be performed before accessing
> > memory pointed by the pointer. Based on the current specification of
> > `addrspacecast` in
> > https://llvm.org/docs/LangRef.html#addrspacecast-to-instruction I am not
> > very clear whether it can be used for this case without any modifications
> > or clarifications and also what would happen if there are multiple AS
> > mismatches. I am going to look at this issue separately in more details. In
> > OpenCL C++ an ICE is triggered for this though. Let me know if you have any
> > thoughts on this.
> Thanks, the check looks good now.
>
> > BTW, while trying to construct a test case for the bug, I have discovered
> > that multiple pointer indirection casting isn't working correctly.
>
> This needs to be an error in Sema. The only qualification conversions that
> should be allowed in general on nested pointers (i.e. on `T` in `T**` or
> `T*&`) are the basic C qualifiers: `const`, `volatile`, and `restrict`; any
> other qualification change there is unsound.
I see. I guess it's because C++ rules don't cover address spaces.
It feels like it would be a regression for OpenCL C++ vs OpenCL C to reject
nested pointers with address spaces because it was allowed before. :(
However, the generation for OpenCL C and C are incorrect currently. I will try
to sort that all out as a separate patch though, if it makes sense?
https://reviews.llvm.org/D53764
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
rjmccall added inline comments.
Comment at: lib/Sema/SemaExprCXX.cpp:4289
+ /*BasePath=*/nullptr, CCK)
+ .get();
Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > Okay. But if `ToType` *isn't* a reference type, this will never be an
> > > > address-space conversion. I feel like this code could be written more
> > > > clearly to express what it's trying to do.
> > > I hope it makes more sense now. Btw, it also applies to pointer type.
> > The logic is wrong for pointer types; if you're converting pointers, you
> > need to be checking the address space of the pointee type of the from type.
> >
> > It sounds like this is totally inadequately tested; please flesh out the
> > test with all of these cases. While you're at it, please ensure that there
> > are tests verifying that we don't allowing address-space changes in nested
> > positions.
> Thanks for spotting this bug! The generated IR for the test was still correct
> because AS of `FromType` happened to correctly mismatch AS of pointee of
> `ToType`.
>
> I failed to construct the test case where it would miss classifying
> `addrspacecast` due to OpenCL or C++ sema rules but I managed to add a case
> in which `addrspacecast` was incorrectly added for pointers where it wasn't
> needed (see line 36 of the test). I think this code is covered now.
>
> As for the address space position in pointers, the following test checks the
> address spaces of pointers in `addrspacecast`. For the other program paths we
> also have a test with similar checks in
> `test/CodeGenOpenCL/address-spaces-conversions.cl` that we now run for C++
> mode too.
>
> BTW, while trying to construct a test case for the bug, I have discovered
> that multiple pointer indirection casting isn't working correctly. I.e. for
> the following program:
> kernel void foo(){
> __private int** loc;
> int** loc_p = loc;
> **loc_p = 1;
> }
> We generate:
> bitcast i32* addrspace(4)* %0 to i32 addrspace(4)* addrspace(4)*
> in OpenCL C and then perform `store` over pointer in AS 4 (generic). We have
> now lost the information that the original pointer was in `private` AS and
> that the adjustment of AS segment has to be performed before accessing memory
> pointed by the pointer. Based on the current specification of `addrspacecast`
> in https://llvm.org/docs/LangRef.html#addrspacecast-to-instruction I am not
> very clear whether it can be used for this case without any modifications or
> clarifications and also what would happen if there are multiple AS
> mismatches. I am going to look at this issue separately in more details. In
> OpenCL C++ an ICE is triggered for this though. Let me know if you have any
> thoughts on this.
Thanks, the check looks good now.
> BTW, while trying to construct a test case for the bug, I have discovered
> that multiple pointer indirection casting isn't working correctly.
This needs to be an error in Sema. The only qualification conversions that
should be allowed in general on nested pointers (i.e. on `T` in `T**` or `T*&`)
are the basic C qualifiers: `const`, `volatile`, and `restrict`; any other
qualification change there is unsound.
https://reviews.llvm.org/D53764
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
Anastasia added inline comments.
Comment at: lib/Sema/SemaExprCXX.cpp:4289
+ /*BasePath=*/nullptr, CCK)
+ .get();
rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Okay. But if `ToType` *isn't* a reference type, this will never be an
> > > address-space conversion. I feel like this code could be written more
> > > clearly to express what it's trying to do.
> > I hope it makes more sense now. Btw, it also applies to pointer type.
> The logic is wrong for pointer types; if you're converting pointers, you need
> to be checking the address space of the pointee type of the from type.
>
> It sounds like this is totally inadequately tested; please flesh out the test
> with all of these cases. While you're at it, please ensure that there are
> tests verifying that we don't allowing address-space changes in nested
> positions.
Thanks for spotting this bug! The generated IR for the test was still correct
because AS of `FromType` happened to correctly mismatch AS of pointee of
`ToType`.
I failed to construct the test case where it would miss classifying
`addrspacecast` due to OpenCL or C++ sema rules but I managed to add a case in
which `addrspacecast` was incorrectly added for pointers where it wasn't needed
(see line 36 of the test). I think this code is covered now.
As for the address space position in pointers, the following test checks the
address spaces of pointers in `addrspacecast`. For the other program paths we
also have a test with similar checks in
`test/CodeGenOpenCL/address-spaces-conversions.cl` that we now run for C++ mode
too.
BTW, while trying to construct a test case for the bug, I have discovered that
multiple pointer indirection casting isn't working correctly. I.e. for the
following program:
kernel void foo(){
__private int** loc;
int** loc_p = loc;
**loc_p = 1;
}
We generate:
bitcast i32* addrspace(4)* %0 to i32 addrspace(4)* addrspace(4)*
in OpenCL C and then perform `store` over pointer in AS 4 (generic). We have
now lost the information that the original pointer was in `private` AS and that
the adjustment of AS segment has to be performed before accessing memory
pointed by the pointer. Based on the current specification of `addrspacecast`
in https://llvm.org/docs/LangRef.html#addrspacecast-to-instruction I am not
very clear whether it can be used for this case without any modifications or
clarifications and also what would happen if there are multiple AS mismatches.
I am going to look at this issue separately in more details. In OpenCL C++ an
ICE is triggered for this though. Let me know if you have any thoughts on this.
https://reviews.llvm.org/D53764
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
Anastasia updated this revision to Diff 174033.
Anastasia added a comment.
Fixed check for AS mismatch of pointer type and added missing test case
https://reviews.llvm.org/D53764
Files:
include/clang/Sema/Sema.h
lib/AST/Expr.cpp
lib/CodeGen/CGExpr.cpp
lib/Sema/DeclSpec.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaInit.cpp
lib/Sema/SemaType.cpp
test/CodeGenOpenCLCXX/address-space-deduction.cl
Index: test/CodeGenOpenCLCXX/address-space-deduction.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/address-space-deduction.cl
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - | FileCheck %s -check-prefixes=COMMON,PTR
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - -DREF | FileCheck %s -check-prefixes=COMMON,REF
+
+#ifdef REF
+#define PTR &
+#define ADR(x) x
+#else
+#define PTR *
+#define ADR(x)
+#endif
+
+//COMMON: @glob = addrspace(1) global i32
+int glob;
+//PTR: @glob_p = addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*)
+//REF: @glob_p = addrspace(1) global i32 addrspace(4)* null
+int PTR glob_p = ADR(glob);
+
+//COMMON: @_ZZ3fooi{{P|R}}U3AS4iE6loc_st = internal addrspace(1) global i32
+//PTR: @_ZZ3fooiPU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @_ZZ3fooiPU3AS4iE6loc_st to i32 addrspace(4)*)
+//REF: @_ZZ3fooiRU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* null
+//COMMON: @loc_ext_p = external addrspace(1) {{global|constant}} i32 addrspace(4)*
+//COMMON: @loc_ext = external addrspace(1) global i32
+
+//REF: store i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*), i32 addrspace(4)* addrspace(1)* @glob_p
+
+//COMMON: define spir_func i32 @_Z3fooi{{P|R}}U3AS4i(i32 %par, i32 addrspace(4)*{{.*}} %par_p)
+int foo(int par, int PTR par_p){
+ //COMMON: %loc = alloca i32
+ int loc;
+ //COMMON: %loc_p = alloca i32 addrspace(4)*
+ //COMMON: %loc_p_const = alloca i32*
+ //COMMON: [[GAS:%[0-9]+]] = addrspacecast i32* %loc to i32 addrspace(4)*
+ //COMMON: store i32 addrspace(4)* [[GAS]], i32 addrspace(4)** %loc_p
+ int PTR loc_p = ADR(loc);
+ //COMMON: store i32* %loc, i32** %loc_p_const
+ const __private int PTR loc_p_const = ADR(loc);
+
+ // CHECK directives for the following code are located above.
+ static int loc_st;
+ //REF: store i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @_ZZ3fooiRU3AS4iE6loc_st to i32 addrspace(4)*), i32 addrspace(4)* addrspace(1)* @_ZZ3fooiRU3AS4iE8loc_st_p
+ static int PTR loc_st_p = ADR(loc_st);
+ extern int loc_ext;
+ extern int PTR loc_ext_p;
+ (void)loc_ext_p;
+ return loc_ext;
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7177,7 +7177,8 @@
bool IsPointee =
ChunkIndex > 0 &&
(D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Pointer ||
- D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer);
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer ||
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Reference);
bool IsFuncReturnType =
ChunkIndex > 0 &&
D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Function;
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -7209,12 +7209,20 @@
return CreateMaterializeTemporaryExpr(E->getType(), E, false);
}
-ExprResult
-InitializationSequence::Perform(Sema ,
-const InitializedEntity ,
-const InitializationKind ,
-MultiExprArg Args,
-QualType *ResultType) {
+ExprResult Sema::PerformQualificationConversion(Expr *E, QualType Ty,
+ExprValueKind VK,
+CheckedConversionKind CCK) {
+ CastKind CK = (Ty.getAddressSpace() != E->getType().getAddressSpace())
+? CK_AddressSpaceConversion
+: CK_NoOp;
+ return ImpCastExprToType(E, Ty, CK, VK, /*BasePath=*/nullptr, CCK);
+}
+
+ExprResult InitializationSequence::Perform(Sema ,
+ const InitializedEntity ,
+ const InitializationKind ,
+ MultiExprArg Args,
+ QualType *ResultType) {
if (Failed()) {
Diagnose(S, Entity, Kind, Args);
return ExprError();
@@ -7603,12 +7611,11 @@
case SK_QualificationConversionRValue: {
// Perform a qualification conversion; these can never go wrong.
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
rjmccall added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:4289 + /*BasePath=*/nullptr, CCK) + .get(); Anastasia wrote: > rjmccall wrote: > > Okay. But if `ToType` *isn't* a reference type, this will never be an > > address-space conversion. I feel like this code could be written more > > clearly to express what it's trying to do. > I hope it makes more sense now. Btw, it also applies to pointer type. The logic is wrong for pointer types; if you're converting pointers, you need to be checking the address space of the pointee type of the from type. It sounds like this is totally inadequately tested; please flesh out the test with all of these cases. While you're at it, please ensure that there are tests verifying that we don't allowing address-space changes in nested positions. https://reviews.llvm.org/D53764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
Anastasia added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:4289 + /*BasePath=*/nullptr, CCK) + .get(); rjmccall wrote: > Okay. But if `ToType` *isn't* a reference type, this will never be an > address-space conversion. I feel like this code could be written more > clearly to express what it's trying to do. I hope it makes more sense now. Btw, it also applies to pointer type. https://reviews.llvm.org/D53764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
Anastasia updated this revision to Diff 173873.
Anastasia added a comment.
Rewrite how CastKind is set for reference and pointer type.
https://reviews.llvm.org/D53764
Files:
include/clang/Sema/Sema.h
lib/AST/Expr.cpp
lib/CodeGen/CGExpr.cpp
lib/Sema/DeclSpec.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaInit.cpp
lib/Sema/SemaType.cpp
test/CodeGenOpenCLCXX/address-space-deduction.cl
Index: test/CodeGenOpenCLCXX/address-space-deduction.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/address-space-deduction.cl
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - | FileCheck %s -check-prefixes=COMMON,PTR
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - -DREF | FileCheck %s -check-prefixes=COMMON,REF
+
+#ifdef REF
+#define PTR &
+#define ADR(x) x
+#else
+#define PTR *
+#define ADR(x)
+#endif
+
+//COMMON: @glob = addrspace(1) global i32
+int glob;
+//PTR: @glob_p = addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*)
+//REF: @glob_p = addrspace(1) global i32 addrspace(4)* null
+int PTR glob_p = ADR(glob);
+
+//COMMON: @_ZZ3fooi{{P|R}}U3AS4iE6loc_st = internal addrspace(1) global i32
+//PTR: @_ZZ3fooiPU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @_ZZ3fooiPU3AS4iE6loc_st to i32 addrspace(4)*)
+//REF: @_ZZ3fooiRU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* null
+//COMMON: @loc_ext_p = external addrspace(1) {{global|constant}} i32 addrspace(4)*
+//COMMON: @loc_ext = external addrspace(1) global i32
+
+//REF: store i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*), i32 addrspace(4)* addrspace(1)* @glob_p
+
+//COMMON: define spir_func i32 @_Z3fooi{{P|R}}U3AS4i(i32 %par, i32 addrspace(4)*{{.*}} %par_p)
+int foo(int par, int PTR par_p){
+//COMMON: %loc = alloca i32
+ int loc;
+//COMMON: %loc_p = alloca i32 addrspace(4)*
+//COMMON: [[GAS:%[0-9]+]] = addrspacecast i32* %loc to i32 addrspace(4)*
+//COMMON: store i32 addrspace(4)* [[GAS]], i32 addrspace(4)** %loc_p
+ int PTR loc_p = ADR(loc);
+
+// CHECK directives for the following code are located above.
+ static int loc_st;
+ static int PTR loc_st_p = ADR(loc_st);
+ extern int loc_ext;
+ extern int PTR loc_ext_p;
+ (void)loc_ext_p;
+ return loc_ext;
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7177,7 +7177,8 @@
bool IsPointee =
ChunkIndex > 0 &&
(D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Pointer ||
- D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer);
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer ||
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Reference);
bool IsFuncReturnType =
ChunkIndex > 0 &&
D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Function;
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -7209,12 +7209,20 @@
return CreateMaterializeTemporaryExpr(E->getType(), E, false);
}
-ExprResult
-InitializationSequence::Perform(Sema ,
-const InitializedEntity ,
-const InitializationKind ,
-MultiExprArg Args,
-QualType *ResultType) {
+ExprResult Sema::PerformQualificationConversion(Expr *E, QualType Ty,
+ExprValueKind VK,
+CheckedConversionKind CCK) {
+ CastKind CK = (Ty.getAddressSpace() != E->getType().getAddressSpace())
+? CK_AddressSpaceConversion
+: CK_NoOp;
+ return ImpCastExprToType(E, Ty, CK, VK, /*BasePath=*/nullptr, CCK);
+}
+
+ExprResult InitializationSequence::Perform(Sema ,
+ const InitializedEntity ,
+ const InitializationKind ,
+ MultiExprArg Args,
+ QualType *ResultType) {
if (Failed()) {
Diagnose(S, Entity, Kind, Args);
return ExprError();
@@ -7603,12 +7611,11 @@
case SK_QualificationConversionRValue: {
// Perform a qualification conversion; these can never go wrong.
ExprValueKind VK =
- Step->Kind == SK_QualificationConversionLValue ?
- VK_LValue :
- (Step->Kind == SK_QualificationConversionXValue ?
- VK_XValue :
- VK_RValue);
- CurInit = S.ImpCastExprToType(CurInit.get(), Step->Type, CK_NoOp, VK);
+ Step->Kind ==
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
rjmccall added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:4289 + /*BasePath=*/nullptr, CCK) + .get(); Okay. But if `ToType` *isn't* a reference type, this will never be an address-space conversion. I feel like this code could be written more clearly to express what it's trying to do. https://reviews.llvm.org/D53764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
Anastasia updated this revision to Diff 173693.
Anastasia marked an inline comment as done.
Anastasia added a comment.
- Extended assert
- Handled AS of ToType
https://reviews.llvm.org/D53764
Files:
include/clang/Sema/Sema.h
lib/AST/Expr.cpp
lib/CodeGen/CGExpr.cpp
lib/Sema/DeclSpec.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaInit.cpp
lib/Sema/SemaType.cpp
test/CodeGenOpenCLCXX/address-space-deduction.cl
Index: test/CodeGenOpenCLCXX/address-space-deduction.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/address-space-deduction.cl
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - | FileCheck %s -check-prefixes=COMMON,PTR
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - -DREF | FileCheck %s -check-prefixes=COMMON,REF
+
+#ifdef REF
+#define PTR &
+#define ADR(x) x
+#else
+#define PTR *
+#define ADR(x)
+#endif
+
+//COMMON: @glob = addrspace(1) global i32
+int glob;
+//PTR: @glob_p = addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*)
+//REF: @glob_p = addrspace(1) global i32 addrspace(4)* null
+int PTR glob_p = ADR(glob);
+
+//COMMON: @_ZZ3fooi{{P|R}}U3AS4iE6loc_st = internal addrspace(1) global i32
+//PTR: @_ZZ3fooiPU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @_ZZ3fooiPU3AS4iE6loc_st to i32 addrspace(4)*)
+//REF: @_ZZ3fooiRU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* null
+//COMMON: @loc_ext_p = external addrspace(1) {{global|constant}} i32 addrspace(4)*
+//COMMON: @loc_ext = external addrspace(1) global i32
+
+//REF: store i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*), i32 addrspace(4)* addrspace(1)* @glob_p
+
+//COMMON: define spir_func i32 @_Z3fooi{{P|R}}U3AS4i(i32 %par, i32 addrspace(4)*{{.*}} %par_p)
+int foo(int par, int PTR par_p){
+//COMMON: %loc = alloca i32
+ int loc;
+//COMMON: %loc_p = alloca i32 addrspace(4)*
+//COMMON: [[GAS:%[0-9]+]] = addrspacecast i32* %loc to i32 addrspace(4)*
+//COMMON: store i32 addrspace(4)* [[GAS]], i32 addrspace(4)** %loc_p
+ int PTR loc_p = ADR(loc);
+
+// CHECK directives for the following code are located above.
+ static int loc_st;
+ static int PTR loc_st_p = ADR(loc_st);
+ extern int loc_ext;
+ extern int PTR loc_ext_p;
+ (void)loc_ext_p;
+ return loc_ext;
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7177,7 +7177,8 @@
bool IsPointee =
ChunkIndex > 0 &&
(D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Pointer ||
- D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer);
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer ||
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Reference);
bool IsFuncReturnType =
ChunkIndex > 0 &&
D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Function;
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -7209,12 +7209,20 @@
return CreateMaterializeTemporaryExpr(E->getType(), E, false);
}
-ExprResult
-InitializationSequence::Perform(Sema ,
-const InitializedEntity ,
-const InitializationKind ,
-MultiExprArg Args,
-QualType *ResultType) {
+ExprResult Sema::PerformQualificationConversion(Expr *E, QualType Ty,
+ExprValueKind VK,
+CheckedConversionKind CCK) {
+ CastKind CK = (Ty.getAddressSpace() != E->getType().getAddressSpace())
+? CK_AddressSpaceConversion
+: CK_NoOp;
+ return ImpCastExprToType(E, Ty, CK, VK, /*BasePath=*/nullptr, CCK);
+}
+
+ExprResult InitializationSequence::Perform(Sema ,
+ const InitializedEntity ,
+ const InitializationKind ,
+ MultiExprArg Args,
+ QualType *ResultType) {
if (Failed()) {
Diagnose(S, Entity, Kind, Args);
return ExprError();
@@ -7603,12 +7611,11 @@
case SK_QualificationConversionRValue: {
// Perform a qualification conversion; these can never go wrong.
ExprValueKind VK =
- Step->Kind == SK_QualificationConversionLValue ?
- VK_LValue :
- (Step->Kind == SK_QualificationConversionXValue ?
- VK_XValue :
- VK_RValue);
- CurInit = S.ImpCastExprToType(CurInit.get(), Step->Type, CK_NoOp, VK);
+
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
rjmccall added inline comments. Comment at: lib/AST/Expr.cpp:1609 case CK_AddressSpaceConversion: -assert(getType()->isPointerType() || getType()->isBlockPointerType()); -assert(getSubExpr()->getType()->isPointerType() || - getSubExpr()->getType()->isBlockPointerType()); -assert(getType()->getPointeeType().getAddressSpace() != - getSubExpr()->getType()->getPointeeType().getAddressSpace()); -LLVM_FALLTHROUGH; +assert(/*If pointer type then addr spaces for pointees must differ*/ + (((getType()->isPointerType() && rjmccall wrote: > rjmccall wrote: > > Anastasia wrote: > > > I don't like this assert now. Would adding extra variable be cleaner here? > > Yeah, this assertion doesn't make any sense like this. It should be > > checking whether the cast is a gl-value and, if so, requiring the > > subexpression to also be a gl-value and then asserting the difference > > between the type. But you can certainly do an address-space conversion on > > l-values that just happen to be of pointer or block-pointer type. > No, if this is a gl-value cast, the assertion must ignore whether there's a > pointee type, or it will be messed up on gl-values of pointer types. > > That is, if I have a gl-value of type `char * __private`, I should be able to > do an address-space promotion to get a gl-value of type `char * __generic`. > It's okay that the pointers are into the same address space here — in fact, > it's more than okay, it's necessary. Thanks, that's right now. Although please assert that the base has the same value kind; I've seen bugs before where ICEs tried to implicitly materialize their arguments, and it's really frustrating to root out. Comment at: lib/Sema/SemaExprCXX.cpp:4285 +? CK_AddressSpaceConversion +: CK_NoOp; + If `ToType` is a reference type, the address space will be on its pointee type. https://reviews.llvm.org/D53764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
Anastasia updated this revision to Diff 173334.
Anastasia added a comment.
Changed the assert for address space conversion.
https://reviews.llvm.org/D53764
Files:
include/clang/Sema/Sema.h
lib/AST/Expr.cpp
lib/CodeGen/CGExpr.cpp
lib/Sema/DeclSpec.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaInit.cpp
lib/Sema/SemaType.cpp
test/CodeGenOpenCLCXX/address-space-deduction.cl
Index: test/CodeGenOpenCLCXX/address-space-deduction.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/address-space-deduction.cl
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - | FileCheck %s -check-prefixes=COMMON,PTR
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - -DREF | FileCheck %s -check-prefixes=COMMON,REF
+
+#ifdef REF
+#define PTR &
+#define ADR(x) x
+#else
+#define PTR *
+#define ADR(x)
+#endif
+
+//COMMON: @glob = addrspace(1) global i32
+int glob;
+//PTR: @glob_p = addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*)
+//REF: @glob_p = addrspace(1) global i32 addrspace(4)* null
+int PTR glob_p = ADR(glob);
+
+//COMMON: @_ZZ3fooi{{P|R}}U3AS4iE6loc_st = internal addrspace(1) global i32
+//PTR: @_ZZ3fooiPU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @_ZZ3fooiPU3AS4iE6loc_st to i32 addrspace(4)*)
+//REF: @_ZZ3fooiRU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* null
+//COMMON: @loc_ext_p = external addrspace(1) {{global|constant}} i32 addrspace(4)*
+//COMMON: @loc_ext = external addrspace(1) global i32
+
+//REF: store i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*), i32 addrspace(4)* addrspace(1)* @glob_p
+
+//COMMON: define spir_func i32 @_Z3fooi{{P|R}}U3AS4i(i32 %par, i32 addrspace(4)*{{.*}} %par_p)
+int foo(int par, int PTR par_p){
+//COMMON: %loc = alloca i32
+ int loc;
+//COMMON: %loc_p = alloca i32 addrspace(4)*
+//COMMON: [[GAS:%[0-9]+]] = addrspacecast i32* %loc to i32 addrspace(4)*
+//COMMON: store i32 addrspace(4)* [[GAS]], i32 addrspace(4)** %loc_p
+ int PTR loc_p = ADR(loc);
+
+// CHECK directives for the following code are located above.
+ static int loc_st;
+ static int PTR loc_st_p = ADR(loc_st);
+ extern int loc_ext;
+ extern int PTR loc_ext_p;
+ (void)loc_ext_p;
+ return loc_ext;
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7177,7 +7177,8 @@
bool IsPointee =
ChunkIndex > 0 &&
(D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Pointer ||
- D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer);
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer ||
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Reference);
bool IsFuncReturnType =
ChunkIndex > 0 &&
D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Function;
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -7209,12 +7209,20 @@
return CreateMaterializeTemporaryExpr(E->getType(), E, false);
}
-ExprResult
-InitializationSequence::Perform(Sema ,
-const InitializedEntity ,
-const InitializationKind ,
-MultiExprArg Args,
-QualType *ResultType) {
+ExprResult Sema::PerformQualificationConversion(Expr *E, QualType Ty,
+ExprValueKind VK,
+CheckedConversionKind CCK) {
+ CastKind CK = (Ty.getAddressSpace() != E->getType().getAddressSpace())
+? CK_AddressSpaceConversion
+: CK_NoOp;
+ return ImpCastExprToType(E, Ty, CK, VK, /*BasePath=*/nullptr, CCK);
+}
+
+ExprResult InitializationSequence::Perform(Sema ,
+ const InitializedEntity ,
+ const InitializationKind ,
+ MultiExprArg Args,
+ QualType *ResultType) {
if (Failed()) {
Diagnose(S, Entity, Kind, Args);
return ExprError();
@@ -7603,12 +7611,11 @@
case SK_QualificationConversionRValue: {
// Perform a qualification conversion; these can never go wrong.
ExprValueKind VK =
- Step->Kind == SK_QualificationConversionLValue ?
- VK_LValue :
- (Step->Kind == SK_QualificationConversionXValue ?
- VK_XValue :
- VK_RValue);
- CurInit = S.ImpCastExprToType(CurInit.get(), Step->Type, CK_NoOp, VK);
+ Step->Kind ==
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
rjmccall added inline comments. Comment at: lib/AST/Expr.cpp:1609 case CK_AddressSpaceConversion: -assert(getType()->isPointerType() || getType()->isBlockPointerType()); -assert(getSubExpr()->getType()->isPointerType() || - getSubExpr()->getType()->isBlockPointerType()); -assert(getType()->getPointeeType().getAddressSpace() != - getSubExpr()->getType()->getPointeeType().getAddressSpace()); -LLVM_FALLTHROUGH; +assert(/*If pointer type then addr spaces for pointees must differ*/ + (((getType()->isPointerType() && rjmccall wrote: > Anastasia wrote: > > I don't like this assert now. Would adding extra variable be cleaner here? > Yeah, this assertion doesn't make any sense like this. It should be checking > whether the cast is a gl-value and, if so, requiring the subexpression to > also be a gl-value and then asserting the difference between the type. But > you can certainly do an address-space conversion on l-values that just happen > to be of pointer or block-pointer type. No, if this is a gl-value cast, the assertion must ignore whether there's a pointee type, or it will be messed up on gl-values of pointer types. That is, if I have a gl-value of type `char * __private`, I should be able to do an address-space promotion to get a gl-value of type `char * __generic`. It's okay that the pointers are into the same address space here — in fact, it's more than okay, it's necessary. https://reviews.llvm.org/D53764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
Anastasia updated this revision to Diff 172109.
Anastasia added a comment.
Addressed comments from John.
https://reviews.llvm.org/D53764
Files:
include/clang/Sema/Sema.h
lib/AST/Expr.cpp
lib/CodeGen/CGExpr.cpp
lib/Sema/DeclSpec.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaInit.cpp
lib/Sema/SemaType.cpp
test/CodeGenOpenCLCXX/address-space-deduction.cl
Index: test/CodeGenOpenCLCXX/address-space-deduction.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/address-space-deduction.cl
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - | FileCheck %s -check-prefixes=COMMON,PTR
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - -DREF | FileCheck %s -check-prefixes=COMMON,REF
+
+#ifdef REF
+#define PTR &
+#define ADR(x) x
+#else
+#define PTR *
+#define ADR(x)
+#endif
+
+//COMMON: @glob = addrspace(1) global i32
+int glob;
+//PTR: @glob_p = addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*)
+//REF: @glob_p = addrspace(1) global i32 addrspace(4)* null
+int PTR glob_p = ADR(glob);
+
+//COMMON: @_ZZ3fooi{{P|R}}U3AS4iE6loc_st = internal addrspace(1) global i32
+//PTR: @_ZZ3fooiPU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @_ZZ3fooiPU3AS4iE6loc_st to i32 addrspace(4)*)
+//REF: @_ZZ3fooiRU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* null
+//COMMON: @loc_ext_p = external addrspace(1) {{global|constant}} i32 addrspace(4)*
+//COMMON: @loc_ext = external addrspace(1) global i32
+
+//REF: store i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*), i32 addrspace(4)* addrspace(1)* @glob_p
+
+//COMMON: define spir_func i32 @_Z3fooi{{P|R}}U3AS4i(i32 %par, i32 addrspace(4)*{{.*}} %par_p)
+int foo(int par, int PTR par_p){
+//COMMON: %loc = alloca i32
+ int loc;
+//COMMON: %loc_p = alloca i32 addrspace(4)*
+//COMMON: [[GAS:%[0-9]+]] = addrspacecast i32* %loc to i32 addrspace(4)*
+//COMMON: store i32 addrspace(4)* [[GAS]], i32 addrspace(4)** %loc_p
+ int PTR loc_p = ADR(loc);
+
+// CHECK directives for the following code are located above.
+ static int loc_st;
+ static int PTR loc_st_p = ADR(loc_st);
+ extern int loc_ext;
+ extern int PTR loc_ext_p;
+ (void)loc_ext_p;
+ return loc_ext;
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7177,7 +7177,8 @@
bool IsPointee =
ChunkIndex > 0 &&
(D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Pointer ||
- D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer);
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer ||
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Reference);
bool IsFuncReturnType =
ChunkIndex > 0 &&
D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Function;
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -7209,12 +7209,20 @@
return CreateMaterializeTemporaryExpr(E->getType(), E, false);
}
-ExprResult
-InitializationSequence::Perform(Sema ,
-const InitializedEntity ,
-const InitializationKind ,
-MultiExprArg Args,
-QualType *ResultType) {
+ExprResult Sema::PerformQualificationConversion(Expr *E, QualType Ty,
+ExprValueKind VK,
+CheckedConversionKind CCK) {
+ CastKind CK = (Ty.getAddressSpace() != E->getType().getAddressSpace())
+? CK_AddressSpaceConversion
+: CK_NoOp;
+ return ImpCastExprToType(E, Ty, CK, VK, /*BasePath=*/nullptr, CCK);
+}
+
+ExprResult InitializationSequence::Perform(Sema ,
+ const InitializedEntity ,
+ const InitializationKind ,
+ MultiExprArg Args,
+ QualType *ResultType) {
if (Failed()) {
Diagnose(S, Entity, Kind, Args);
return ExprError();
@@ -7603,12 +7611,11 @@
case SK_QualificationConversionRValue: {
// Perform a qualification conversion; these can never go wrong.
ExprValueKind VK =
- Step->Kind == SK_QualificationConversionLValue ?
- VK_LValue :
- (Step->Kind == SK_QualificationConversionXValue ?
- VK_XValue :
- VK_RValue);
- CurInit = S.ImpCastExprToType(CurInit.get(), Step->Type, CK_NoOp, VK);
+ Step->Kind == SK_QualificationConversionLValue
+
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
rjmccall added inline comments.
Comment at: lib/AST/Expr.cpp:1609
case CK_AddressSpaceConversion:
-assert(getType()->isPointerType() || getType()->isBlockPointerType());
-assert(getSubExpr()->getType()->isPointerType() ||
- getSubExpr()->getType()->isBlockPointerType());
-assert(getType()->getPointeeType().getAddressSpace() !=
- getSubExpr()->getType()->getPointeeType().getAddressSpace());
-LLVM_FALLTHROUGH;
+assert(/*If pointer type then addr spaces for pointees must differ*/
+ (((getType()->isPointerType() &&
Anastasia wrote:
> I don't like this assert now. Would adding extra variable be cleaner here?
Yeah, this assertion doesn't make any sense like this. It should be checking
whether the cast is a gl-value and, if so, requiring the subexpression to also
be a gl-value and then asserting the difference between the type. But you can
certainly do an address-space conversion on l-values that just happen to be of
pointer or block-pointer type.
Comment at: lib/CodeGen/CGExpr.cpp:4252
+Address V =
+Builder.CreateAddrSpaceCast(LV.getAddress(), ConvertType(DestTy));
+
Please use the `performAddrSpaceCast` target hook instead of directly
constructing an LLVM `addrspacecast`.
Comment at: lib/Sema/DeclSpec.cpp:576
+ if (S.getLangOpts().OpenCLVersion < 120 &&
+ !S.getLangOpts().OpenCLCPlusPlus) {
+DiagID = diag::err_opencl_unknown_type_specifier;
Please update the comment above this.
Comment at: lib/Sema/SemaDecl.cpp:7366
+ (getLangOpts().OpenCLVersion == 200 ||
+ getLangOpts().OpenCLCPlusPlus {
int Scope = NewVD->isStaticLocal() | NewVD->hasExternalStorage() << 1;
Please update the comment above this.
Comment at: lib/Sema/SemaInit.cpp:7614
+: CK_NoOp;
+ CurInit = S.ImpCastExprToType(CurInit.get(), Step->Type, CK, VK);
break;
Please extract a function to do an l-value qualification conversion just in
case we add more non-trivial conversions that we need to represent.
https://reviews.llvm.org/D53764
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
Anastasia added inline comments. Comment at: lib/AST/Expr.cpp:1609 case CK_AddressSpaceConversion: -assert(getType()->isPointerType() || getType()->isBlockPointerType()); -assert(getSubExpr()->getType()->isPointerType() || - getSubExpr()->getType()->isBlockPointerType()); -assert(getType()->getPointeeType().getAddressSpace() != - getSubExpr()->getType()->getPointeeType().getAddressSpace()); -LLVM_FALLTHROUGH; +assert(/*If pointer type then addr spaces for pointees must differ*/ + (((getType()->isPointerType() && I don't like this assert now. Would adding extra variable be cleaner here? https://reviews.llvm.org/D53764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53764: [OpenCL] Enable address spaces for references in C++
Anastasia created this revision.
Anastasia added reviewers: rjmccall, yaxunl.
I first enabled AS deduction for references that allowed to inherit the right
conversion diagnostics based on qualification conversion rules implemented
earlier for the pointer type.
Then in order to tests the deduction rules fully, I had to enable some extra
features from OpenCL 2.0 that are also valid in C++.
A number of ICEs fired in the `CodeGen` due to missing `addrspacecast`. Not
convinced the current solution is good though. May be it would be cleaner to
add a separate `CastKind` here - `CK_LValueAddressSpaceConversion`? Although I
am not entirely clear about the benefits yet.
https://reviews.llvm.org/D53764
Files:
lib/AST/Expr.cpp
lib/CodeGen/CGExpr.cpp
lib/Sema/DeclSpec.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaInit.cpp
lib/Sema/SemaType.cpp
test/CodeGenOpenCLCXX/address-space-deduction.cl
Index: test/CodeGenOpenCLCXX/address-space-deduction.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/address-space-deduction.cl
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - | FileCheck %s -check-prefixes=COMMON,PTR
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - -DREF | FileCheck %s -check-prefixes=COMMON,REF
+
+#ifdef REF
+#define PTR &
+#define ADR(x) x
+#else
+#define PTR *
+#define ADR(x)
+#endif
+
+//COMMON: @glob = addrspace(1) global i32
+int glob;
+//PTR: @glob_p = addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*)
+//REF: @glob_p = addrspace(1) global i32 addrspace(4)* null
+int PTR glob_p = ADR(glob);
+
+//COMMON: @_ZZ3fooi{{P|R}}U3AS4iE6loc_st = internal addrspace(1) global i32
+//PTR: @_ZZ3fooiPU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @_ZZ3fooiPU3AS4iE6loc_st to i32 addrspace(4)*)
+//REF: @_ZZ3fooiRU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* null
+//COMMON: @loc_ext_p = external addrspace(1) {{global|constant}} i32 addrspace(4)*
+//COMMON: @loc_ext = external addrspace(1) global i32
+
+//REF: store i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*), i32 addrspace(4)* addrspace(1)* @glob_p
+
+//COMMON: define spir_func i32 @_Z3fooi{{P|R}}U3AS4i(i32 %par, i32 addrspace(4)*{{.*}} %par_p)
+int foo(int par, int PTR par_p){
+//COMMON: %loc = alloca i32
+ int loc;
+//COMMON: %loc_p = alloca i32 addrspace(4)*
+//COMMON: [[GAS:%[0-9]+]] = addrspacecast i32* %loc to i32 addrspace(4)*
+//COMMON: store i32 addrspace(4)* [[GAS]], i32 addrspace(4)** %loc_p
+ int PTR loc_p = ADR(loc);
+
+// CHECK directives for the following code are located above.
+ static int loc_st;
+ static int PTR loc_st_p = ADR(loc_st);
+ extern int loc_ext;
+ extern int PTR loc_ext_p;
+ (void)loc_ext_p;
+ return loc_ext;
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7177,7 +7177,8 @@
bool IsPointee =
ChunkIndex > 0 &&
(D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Pointer ||
- D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer);
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer ||
+ D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Reference);
bool IsFuncReturnType =
ChunkIndex > 0 &&
D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Function;
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -7603,12 +7603,15 @@
case SK_QualificationConversionRValue: {
// Perform a qualification conversion; these can never go wrong.
ExprValueKind VK =
- Step->Kind == SK_QualificationConversionLValue ?
- VK_LValue :
- (Step->Kind == SK_QualificationConversionXValue ?
- VK_XValue :
- VK_RValue);
- CurInit = S.ImpCastExprToType(CurInit.get(), Step->Type, CK_NoOp, VK);
+ Step->Kind == SK_QualificationConversionLValue
+ ? VK_LValue
+ : (Step->Kind == SK_QualificationConversionXValue ? VK_XValue
+: VK_RValue);
+ CastKind CK = (Step->Type.getAddressSpace() !=
+ CurInit.get()->getType().getAddressSpace())
+? CK_AddressSpaceConversion
+: CK_NoOp;
+ CurInit = S.ImpCastExprToType(CurInit.get(), Step->Type, CK, VK);
break;
}
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -4276,10 +4276,17 @@
case
