[PATCH] D58634: [PR40778] Generate address space conversion when binding reference to a temporary value in different address space
Anastasia added inline comments.
Comment at: lib/Sema/SemaInit.cpp:4806-4808
+ if ((RefRelationship == Sema::Ref_Related &&
+ (T1CVRQuals | T2CVRQuals) != T1CVRQuals) ||
+ !T1Quals.isAddressSpaceSupersetOf(T2Quals)) {
ebevhan wrote:
> Sorry for the really late comment on this, but shouldn't this be:
> ```
> if (RefRelationship == Sema::Ref_Related &&
> ((T1CVRQuals | T2CVRQuals) != T1CVRQuals ||
>!T1Quals.isAddressSpaceSupersetOf(T2Quals))) {
> ```
>
> Currently, this fails on AS qualification regardless of ref-compatibility.
Sorry for noticing this late. I agree this doesn't look right... I will create
a patch to fix this.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58634/new/
https://reviews.llvm.org/D58634
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58634: [PR40778] Generate address space conversion when binding reference to a temporary value in different address space
ebevhan added inline comments.
Comment at: lib/Sema/SemaInit.cpp:4806-4808
+ if ((RefRelationship == Sema::Ref_Related &&
+ (T1CVRQuals | T2CVRQuals) != T1CVRQuals) ||
+ !T1Quals.isAddressSpaceSupersetOf(T2Quals)) {
Sorry for the really late comment on this, but shouldn't this be:
```
if (RefRelationship == Sema::Ref_Related &&
((T1CVRQuals | T2CVRQuals) != T1CVRQuals ||
!T1Quals.isAddressSpaceSupersetOf(T2Quals))) {
```
Currently, this fails on AS qualification regardless of ref-compatibility.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58634/new/
https://reviews.llvm.org/D58634
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58634: [PR40778] Generate address space conversion when binding reference to a temporary value in different address space
This revision was automatically updated to reflect the committed changes.
Closed by commit rC355499: [PR40778] Add addr space conversion when binding
reference to a temporary. (authored by stulova, committed by ).
Herald added a project: clang.
Changed prior to commit:
https://reviews.llvm.org/D58634?vs=189313=189485#toc
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58634/new/
https://reviews.llvm.org/D58634
Files:
include/clang/AST/Type.h
lib/Sema/SemaInit.cpp
test/CodeGenOpenCLCXX/addrspace-references.cl
Index: include/clang/AST/Type.h
===
--- include/clang/AST/Type.h
+++ include/clang/AST/Type.h
@@ -317,6 +317,11 @@
qs.removeObjCLifetime();
return qs;
}
+ Qualifiers withoutAddressSpace() const {
+Qualifiers qs = *this;
+qs.removeAddressSpace();
+return qs;
+ }
bool hasObjCLifetime() const { return Mask & LifetimeMask; }
ObjCLifetime getObjCLifetime() const {
Index: test/CodeGenOpenCLCXX/addrspace-references.cl
===
--- test/CodeGenOpenCLCXX/addrspace-references.cl
+++ test/CodeGenOpenCLCXX/addrspace-references.cl
@@ -0,0 +1,14 @@
+//RUN: %clang_cc1 %s -cl-std=c++ -triple spir -emit-llvm -o - | FileCheck %s
+
+int bar(const unsigned int );
+// CHECK-LABEL: define spir_func void @_Z3foov()
+void foo() {
+ // The generic addr space reference parameter object will be bound
+ // to a temporary value allocated in private addr space. We need an
+ // addrspacecast before passing the value to the function.
+ // CHECK: [[REF:%.*]] = alloca i32
+ // CHECK: store i32 1, i32* [[REF]]
+ // CHECK: [[REG:%[0-9]+]] = addrspacecast i32* [[REF]] to i32 addrspace(4)*
+ // CHECK: call spir_func i32 @_Z3barRU3AS4Kj(i32 addrspace(4)* nonnull
dereferenceable(4) [[REG]])
+ bar(1);
+}
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -4760,7 +4760,15 @@
//copy-initialization (8.5). The reference is then bound to the
//temporary. [...]
- InitializedEntity TempEntity = InitializedEntity::InitializeTemporary(cv1T1);
+ // Ignore address space of reference type at this point and perform address
+ // space conversion after the reference binding step.
+ QualType cv1T1IgnoreAS =
+ T1Quals.hasAddressSpace()
+ ? S.Context.getQualifiedType(T1, T1Quals.withoutAddressSpace())
+ : cv1T1;
+
+ InitializedEntity TempEntity =
+ InitializedEntity::InitializeTemporary(cv1T1IgnoreAS);
// FIXME: Why do we use an implicit conversion here rather than trying
// copy-initialization?
@@ -4795,8 +4803,9 @@
//than, cv2; otherwise, the program is ill-formed.
unsigned T1CVRQuals = T1Quals.getCVRQualifiers();
unsigned T2CVRQuals = T2Quals.getCVRQualifiers();
- if (RefRelationship == Sema::Ref_Related &&
- (T1CVRQuals | T2CVRQuals) != T1CVRQuals) {
+ if ((RefRelationship == Sema::Ref_Related &&
+ (T1CVRQuals | T2CVRQuals) != T1CVRQuals) ||
+ !T1Quals.isAddressSpaceSupersetOf(T2Quals)) {
Sequence.SetFailed(InitializationSequence::FK_ReferenceInitDropsQualifiers);
return;
}
@@ -4810,7 +4819,11 @@
return;
}
- Sequence.AddReferenceBindingStep(cv1T1, /*bindingTemporary=*/true);
+ Sequence.AddReferenceBindingStep(cv1T1IgnoreAS, /*bindingTemporary=*/true);
+
+ if (T1Quals.hasAddressSpace())
+Sequence.AddQualificationConversionStep(cv1T1, isLValueRef ? VK_LValue
+ : VK_XValue);
}
/// Attempt character array initialization from a string literal
Index: include/clang/AST/Type.h
===
--- include/clang/AST/Type.h
+++ include/clang/AST/Type.h
@@ -317,6 +317,11 @@
qs.removeObjCLifetime();
return qs;
}
+ Qualifiers withoutAddressSpace() const {
+Qualifiers qs = *this;
+qs.removeAddressSpace();
+return qs;
+ }
bool hasObjCLifetime() const { return Mask & LifetimeMask; }
ObjCLifetime getObjCLifetime() const {
Index: test/CodeGenOpenCLCXX/addrspace-references.cl
===
--- test/CodeGenOpenCLCXX/addrspace-references.cl
+++ test/CodeGenOpenCLCXX/addrspace-references.cl
@@ -0,0 +1,14 @@
+//RUN: %clang_cc1 %s -cl-std=c++ -triple spir -emit-llvm -o - | FileCheck %s
+
+int bar(const unsigned int );
+// CHECK-LABEL: define spir_func void @_Z3foov()
+void foo() {
+ // The generic addr space reference parameter object will be bound
+ // to a temporary value allocated in private addr space. We need an
+ // addrspacecast before passing the value to the function.
+ // CHECK: [[REF:%.*]] = alloca i32
+ // CHECK: store i32 1, i32* [[REF]]
+ // CHECK: [[REG:%[0-9]+]] = addrspacecast i32* [[REF]] to i32 addrspace(4)*
+
[PATCH] D58634: [PR40778] Generate address space conversion when binding reference to a temporary value in different address space
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. This test file will probably grow over time, so please add a CHECK-LABEL line to the test case to make sure you're checking the body of foo(). Also, you'll probably need to allow the name `%ref.tmp` to be stripped for this test to pass in release builds; please go ahead and test a more complete pattern, including the alloca and the store of 1 to it. Otherwise LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58634/new/ https://reviews.llvm.org/D58634 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58634: [PR40778] Generate address space conversion when binding reference to a temporary value in different address space
Anastasia updated this revision to Diff 189313.
Anastasia added a comment.
- Implement the fix correctly by added an extra address space conversion step
after binding the reference
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58634/new/
https://reviews.llvm.org/D58634
Files:
include/clang/AST/Type.h
lib/Sema/SemaInit.cpp
test/CodeGenOpenCLCXX/addrspace-references.cl
Index: test/CodeGenOpenCLCXX/addrspace-references.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-references.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_cc1 %s -cl-std=c++ -triple spir -emit-llvm -o - | FileCheck %s
+
+int bar(const unsigned int );
+
+void foo() {
+ // The generic addr space reference parameter object will be bound
+ // to a temporary value allocated in private addr space. We need an
+ // addrspacecast before passing the value to the function.
+ // CHECK: addrspacecast i32* %ref.tmp to i32 addrspace(4)*
+ bar(1);
+}
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -4760,7 +4760,15 @@
//copy-initialization (8.5). The reference is then bound to the
//temporary. [...]
- InitializedEntity TempEntity = InitializedEntity::InitializeTemporary(cv1T1);
+ // Ignore address space of reference type at this point and perform address
+ // space conversion after the reference binding step.
+ QualType cv1T1IgnoreAS =
+ T1Quals.hasAddressSpace()
+ ? S.Context.getQualifiedType(T1, T1Quals.withoutAddressSpace())
+ : cv1T1;
+
+ InitializedEntity TempEntity =
+ InitializedEntity::InitializeTemporary(cv1T1IgnoreAS);
// FIXME: Why do we use an implicit conversion here rather than trying
// copy-initialization?
@@ -4795,8 +4803,9 @@
//than, cv2; otherwise, the program is ill-formed.
unsigned T1CVRQuals = T1Quals.getCVRQualifiers();
unsigned T2CVRQuals = T2Quals.getCVRQualifiers();
- if (RefRelationship == Sema::Ref_Related &&
- (T1CVRQuals | T2CVRQuals) != T1CVRQuals) {
+ if ((RefRelationship == Sema::Ref_Related &&
+ (T1CVRQuals | T2CVRQuals) != T1CVRQuals) ||
+ !T1Quals.isAddressSpaceSupersetOf(T2Quals)) {
Sequence.SetFailed(InitializationSequence::FK_ReferenceInitDropsQualifiers);
return;
}
@@ -4810,7 +4819,11 @@
return;
}
- Sequence.AddReferenceBindingStep(cv1T1, /*bindingTemporary=*/true);
+ Sequence.AddReferenceBindingStep(cv1T1IgnoreAS, /*bindingTemporary=*/true);
+
+ if (T1Quals.hasAddressSpace())
+Sequence.AddQualificationConversionStep(cv1T1, isLValueRef ? VK_LValue
+ : VK_XValue);
}
/// Attempt character array initialization from a string literal
Index: include/clang/AST/Type.h
===
--- include/clang/AST/Type.h
+++ include/clang/AST/Type.h
@@ -317,6 +317,11 @@
qs.removeObjCLifetime();
return qs;
}
+ Qualifiers withoutAddressSpace() const {
+Qualifiers qs = *this;
+qs.removeAddressSpace();
+return qs;
+ }
bool hasObjCLifetime() const { return Mask & LifetimeMask; }
ObjCLifetime getObjCLifetime() const {
Index: test/CodeGenOpenCLCXX/addrspace-references.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-references.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_cc1 %s -cl-std=c++ -triple spir -emit-llvm -o - | FileCheck %s
+
+int bar(const unsigned int );
+
+void foo() {
+ // The generic addr space reference parameter object will be bound
+ // to a temporary value allocated in private addr space. We need an
+ // addrspacecast before passing the value to the function.
+ // CHECK: addrspacecast i32* %ref.tmp to i32 addrspace(4)*
+ bar(1);
+}
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -4760,7 +4760,15 @@
//copy-initialization (8.5). The reference is then bound to the
//temporary. [...]
- InitializedEntity TempEntity = InitializedEntity::InitializeTemporary(cv1T1);
+ // Ignore address space of reference type at this point and perform address
+ // space conversion after the reference binding step.
+ QualType cv1T1IgnoreAS =
+ T1Quals.hasAddressSpace()
+ ? S.Context.getQualifiedType(T1, T1Quals.withoutAddressSpace())
+ : cv1T1;
+
+ InitializedEntity TempEntity =
+ InitializedEntity::InitializeTemporary(cv1T1IgnoreAS);
// FIXME: Why do we use an implicit conversion here rather than trying
// copy-initialization?
@@ -4795,8 +4803,9 @@
//than, cv2; otherwise, the program is ill-formed.
unsigned T1CVRQuals = T1Quals.getCVRQualifiers();
unsigned T2CVRQuals = T2Quals.getCVRQualifiers();
- if (RefRelationship ==
[PATCH] D58634: [PR40778] Generate address space conversion when binding reference to a temporary value in different address space
Anastasia marked an inline comment as done. Anastasia added inline comments. Comment at: lib/CodeGen/CGCall.cpp:4067 + IRFuncTy->getParamType(FirstIRArg)->isPointerTy()) +V = Builder.CreatePointerBitCastOrAddrSpaceCast( +V, IRFuncTy->getParamType(FirstIRArg)); rjmccall wrote: > Anastasia wrote: > > We have started using `performAddrSpaceCast` for those but, however, I was > > very confused about how to get address space of the Clang types correctly > > here. > > > > I was using this function in a couple of places before but I must admit I > > am still a bit confused about the purpose of it. Mainly I was wondering if > > we could drop `LangAS` parameters from it? It would simplify its use a lot > > in multiple places. As far as I can see they are not used in the function > > at the moment. I am not sure if they are reserved for some development in > > the future though? > > > > Altogether I quite like using `IRBuilder` directly because in combination > > with target address space map it allows to do what's needed here and keep > > consistent with the rest. Perhaps, I am missing some background info. I > > have tried to dig out a bit but no much success. > We want to allow language address spaces to be arbitrarily mapped to IR > address spaces during lowering, which might include e.g. changing the null > value. That's why we pass language address spaces down. > > You should be getting the language address space from the appropriate type. > I wouldn't expect that to include address-space conversions at this point, > though; is there maybe a missing implicit conversions somewhere? > We want to allow language address spaces to be arbitrarily mapped to IR > address spaces during lowering, which might include e.g. changing the null > value. That's why we pass language address spaces down. Would this mean we can map AST address spaces into different IR ones from what is the the target address space map? Ok right, null pointer value can't be handled by the target address space map at all. I am a bit confused though why we are not using these LangAS at the moment. I guess we just don't have upstream targets requiring that... > is there maybe a missing implicit conversions somewhere? Ok, the AST dump doesn't seem right for the test case I included. `MaterializeTemporaryExpr` is in `generic` addr space. I guess it should be `private`... and then converted to `generic`. ``` `-FunctionDecl 0x712608 line:5:6 foo 'void ()' `-CompoundStmt 0x712810 `-ExprWithCleanups 0x7127f8 'int' `-CallExpr 0x7127a0 'int' |-ImplicitCastExpr 0x712788 'int (*)(const __generic unsigned int &)' | `-DeclRefExpr 0x712710 'int (const __generic unsigned int &)' lvalue Function 0x7124d0 'bar' 'int (const __generic unsigned int &)' `-MaterializeTemporaryExpr 0x7127e0 'const __generic unsigned int' lvalue `-ImplicitCastExpr 0x7127c8 'const __generic unsigned int' `-IntegerLiteral 0x7126f0 'int' 1 ``` I will fix it! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58634/new/ https://reviews.llvm.org/D58634 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58634: [PR40778] Generate address space conversion when binding reference to a temporary value in different address space
rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:4067 + IRFuncTy->getParamType(FirstIRArg)->isPointerTy()) +V = Builder.CreatePointerBitCastOrAddrSpaceCast( +V, IRFuncTy->getParamType(FirstIRArg)); Anastasia wrote: > We have started using `performAddrSpaceCast` for those but, however, I was > very confused about how to get address space of the Clang types correctly > here. > > I was using this function in a couple of places before but I must admit I am > still a bit confused about the purpose of it. Mainly I was wondering if we > could drop `LangAS` parameters from it? It would simplify its use a lot in > multiple places. As far as I can see they are not used in the function at the > moment. I am not sure if they are reserved for some development in the future > though? > > Altogether I quite like using `IRBuilder` directly because in combination > with target address space map it allows to do what's needed here and keep > consistent with the rest. Perhaps, I am missing some background info. I have > tried to dig out a bit but no much success. We want to allow language address spaces to be arbitrarily mapped to IR address spaces during lowering, which might include e.g. changing the null value. That's why we pass language address spaces down. You should be getting the language address space from the appropriate type. I wouldn't expect that to include address-space conversions at this point, though; is there maybe a missing implicit conversions somewhere? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58634/new/ https://reviews.llvm.org/D58634 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58634: [PR40778] Generate address space conversion when binding reference to a temporary value in different address space
Anastasia marked an inline comment as done. Anastasia added inline comments. Comment at: lib/CodeGen/CGCall.cpp:4067 + IRFuncTy->getParamType(FirstIRArg)->isPointerTy()) +V = Builder.CreatePointerBitCastOrAddrSpaceCast( +V, IRFuncTy->getParamType(FirstIRArg)); We have started using `performAddrSpaceCast` for those but, however, I was very confused about how to get address space of the Clang types correctly here. I was using this function in a couple of places before but I must admit I am still a bit confused about the purpose of it. Mainly I was wondering if we could drop `LangAS` parameters from it? It would simplify its use a lot in multiple places. As far as I can see they are not used in the function at the moment. I am not sure if they are reserved for some development in the future though? Altogether I quite like using `IRBuilder` directly because in combination with target address space map it allows to do what's needed here and keep consistent with the rest. Perhaps, I am missing some background info. I have tried to dig out a bit but no much success. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58634/new/ https://reviews.llvm.org/D58634 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58634: [PR40778] Generate address space conversion when binding reference to a temporary value in different address space
Anastasia created this revision.
Anastasia added reviewers: rjmccall, ebevhan.
This is fixing one of the issues reported in the bug:
https://bugs.llvm.org/show_bug.cgi?id=40778
https://reviews.llvm.org/D58634
Files:
lib/CodeGen/CGCall.cpp
test/CodeGenOpenCLCXX/addrspace-references.cl
Index: test/CodeGenOpenCLCXX/addrspace-references.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-references.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_cc1 %s -cl-std=c++ -triple spir -emit-llvm -o - | FileCheck %s
+
+int bar(const unsigned int );
+
+void foo() {
+ // The generic addr space reference parameter object will be bound
+ // to a temporary value allocated in private addr space. We need an
+ // addrspacecast before passing the value to the function.
+ // CHECK: addrspacecast i32* %ref.tmp to i32 addrspace(4)*
+ bar(1);
+}
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4058,12 +4058,16 @@
if (ArgInfo.getCoerceToType() != V->getType() &&
V->getType()->isIntegerTy())
V = Builder.CreateZExt(V, ArgInfo.getCoerceToType());
-
// If the argument doesn't match, perform a bitcast to coerce it. This
// can happen due to trivial type mismatches.
if (FirstIRArg < IRFuncTy->getNumParams() &&
V->getType() != IRFuncTy->getParamType(FirstIRArg))
- V = Builder.CreateBitCast(V, IRFuncTy->getParamType(FirstIRArg));
+ if (V->getType()->isPointerTy() &&
+ IRFuncTy->getParamType(FirstIRArg)->isPointerTy())
+V = Builder.CreatePointerBitCastOrAddrSpaceCast(
+V, IRFuncTy->getParamType(FirstIRArg));
+ else
+V = Builder.CreateBitCast(V, IRFuncTy->getParamType(FirstIRArg));
IRCallArgs[FirstIRArg] = V;
break;
Index: test/CodeGenOpenCLCXX/addrspace-references.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-references.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_cc1 %s -cl-std=c++ -triple spir -emit-llvm -o - | FileCheck %s
+
+int bar(const unsigned int );
+
+void foo() {
+ // The generic addr space reference parameter object will be bound
+ // to a temporary value allocated in private addr space. We need an
+ // addrspacecast before passing the value to the function.
+ // CHECK: addrspacecast i32* %ref.tmp to i32 addrspace(4)*
+ bar(1);
+}
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4058,12 +4058,16 @@
if (ArgInfo.getCoerceToType() != V->getType() &&
V->getType()->isIntegerTy())
V = Builder.CreateZExt(V, ArgInfo.getCoerceToType());
-
// If the argument doesn't match, perform a bitcast to coerce it. This
// can happen due to trivial type mismatches.
if (FirstIRArg < IRFuncTy->getNumParams() &&
V->getType() != IRFuncTy->getParamType(FirstIRArg))
- V = Builder.CreateBitCast(V, IRFuncTy->getParamType(FirstIRArg));
+ if (V->getType()->isPointerTy() &&
+ IRFuncTy->getParamType(FirstIRArg)->isPointerTy())
+V = Builder.CreatePointerBitCastOrAddrSpaceCast(
+V, IRFuncTy->getParamType(FirstIRArg));
+ else
+V = Builder.CreateBitCast(V, IRFuncTy->getParamType(FirstIRArg));
IRCallArgs[FirstIRArg] = V;
break;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
