[PATCH] D62574: Add support for target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Sorry for the delay! I rebased the patch onto latest and it mostly worked, with 
a few hitches.




Comment at: clang/lib/Sema/SemaCast.cpp:2617-2619
+  (Self.getLangOpts().OpenCL &&
+   (DestPPointee->isFunctionType() ||
+SrcPPointee->isFunctionType() {

Without this, `SemaOpenCL/func.cl` fails since `foo((void*)foo)` is no longer 
an error. Since the function pointer is Default-qualified, 
`isExplicitAddrSpaceConversionLegal` delegates the check to the target, which 
will always return true.

To be honest, the clause in that method for letting targets allow conversions 
where `LangAS::Default` is involved only works so long as every type in OpenCL 
has the "right" semantic AS. When any of them are Default, things like this 
happen.

Maybe we should only ask the target about explicit conversions if both ASes are 
Default or a target AS?



Comment at: clang/lib/Sema/SemaOverload.cpp:5289
 
+  // FIXME: hasAddressSpace is wrong; this check will be skipped if FromType is
+  // not qualified with an address space, but if there's no implicit conversion

Anastasia wrote:
> ebevhan wrote:
> > Anastasia wrote:
> > > Do you have a failing test case, if so feel free to create a bug?
> > Unsure how I'd make one. I suspect this can't be triggered in OpenCL++, 
> > because you can't really have LangAS::Default on FromType there, can you? 
> > It would always be some AS.
> > 
> > Doing it in another way would require a target that has configurable ASes, 
> > which doesn't exist yet. Also, it would require methods qualified with 
> > target ASes, and that doesn't work yet either.
> Ok, that's right in OpenCL almost every type gets an address space early in 
> parsing.
> 
> But if we already know it's a bug and the fix for it we could change this 
> code? Although I think the bug fix should better be done on a separate review.
I made the fix here just to try, and it caused a difference in 
address-space-lambda.clcpp.

I'm not sure why that lambda would be valid just because `mutable` is added... 
So this seems like the correct behavior to me?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62574/new/

https://reviews.llvm.org/D62574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62574: Add support for target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 444638.
ebevhan added a comment.
Herald added a subscriber: steakhal.

Rebased and addressed some comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62574/new/

https://reviews.llvm.org/D62574

Files:
  clang-tools-extra/clang-tidy/bugprone/VirtualNearMissCheck.cpp
  clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/CanonicalType.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaFixItUtils.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/test/Analysis/addrspace-null.c
  clang/test/CodeGenCXX/address-space-cast.cpp
  clang/test/Sema/address_space_print_macro.c
  clang/test/Sema/address_spaces.c
  clang/test/SemaOpenCLCXX/address-space-lambda.clcpp

Index: clang/test/SemaOpenCLCXX/address-space-lambda.clcpp
===
--- clang/test/SemaOpenCLCXX/address-space-lambda.clcpp
+++ clang/test/SemaOpenCLCXX/address-space-lambda.clcpp
@@ -66,7 +66,7 @@
 // expected-warning@-2{{lambda without a parameter clause is a C++2b extension}}
 #endif
 
-  [&] () mutable __private {} ();
+  [&] () mutable __private {} (); // expected-error{{no matching function for call to object of type '(lambda at}} expected-note{{candidate function not viable: 'this' object is in default address space, but method expects object in address space '__private'}}
   [&] () __private mutable {} (); //expected-error{{expected body of lambda expression}}
 }
 
Index: clang/test/Sema/address_spaces.c
===
--- clang/test/Sema/address_spaces.c
+++ clang/test/Sema/address_spaces.c
@@ -71,7 +71,8 @@
 
 // Clang extension doesn't forbid operations on pointers to different address spaces.
 char* cmp(_AS1 char *x,  _AS2 char *y) {
-  return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('_AS1 char *' and '_AS2 char *') which are pointers to non-overlapping address spaces}}
+  return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('_AS1 char *' and '_AS2 char *') which are pointers to non-overlapping address spaces}} \
+// expected-error{{comparison between  ('_AS1 char *' and '_AS2 char *') which are pointers to non-overlapping address spaces}}
 }
 
 char *sub(_AS1 char *x, _AS2 char *y) {
Index: clang/test/Sema/address_space_print_macro.c
===
--- clang/test/Sema/address_space_print_macro.c
+++ clang/test/Sema/address_space_print_macro.c
@@ -14,7 +14,8 @@
 }
 
 char *cmp(AS1 char *x, AS2 char *y) {
-  return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('AS1 char *' and 'AS2 char *') which are pointers to non-overlapping address spaces}}
+  return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('AS1 char *' and 'AS2 char *') which are pointers to non-overlapping address spaces}} \
+// expected-error{{comparison between  ('AS1 char *' and 'AS2 char *') which are pointers to non-overlapping address spaces}}
 }
 
 __attribute__((address_space(1))) char test_array[10];
Index: clang/test/CodeGenCXX/address-space-cast.cpp
===
--- clang/test/CodeGenCXX/address-space-cast.cpp
+++ clang/test/CodeGenCXX/address-space-cast.cpp
@@ -37,8 +37,9 @@
   // CHECK-NEXT: store i8 addrspace(5)* %[[cast]]
   priv_void_ptr = (__private__ void *)gen_void_ptr;
 
-  // CHECK: %[[cast:.*]] = addrspacecast i32* %{{.*}} to i8 addrspace(5)*
-  // CHECK-NEXT: store i8 addrspace(5)* %[[cast]]
+  // CHECK: %[[cast:.*]] = bitcast i32* %{{.*}} to i8*
+  // CHECK-NEXT: %[[cast2:.*]] = addrspacecast i8* %[[cast]] to i8 addrspace(5)*
+  // CHECK-NEXT: store i8 addrspace(5)* %[[cast2]]
   priv_void_ptr = (__private__ void *)gen_int_ptr;
 
   // CHECK: %[[cast:.*]] = addrspacecast i8* %{{.*}} to i32 addrspace(5)*
@@ -65,8 +66,9 @@
   // CHECK-NEXT: call void @_Z10func_pvoidPU3AS5v(i8 addrspace(5)* noundef %[[cast]])
   func_pvoid((__private__ void *)gen_void_ptr);
 
-  // CHECK: %[[cast:.*]] = addrspacecast i32* %{{.*}} to i8 addrspace(5)*
-  // CHECK-NEXT: call void @_Z10func_pvoidPU3AS5v(i8 addrspace(5)* noundef %[[cast]])
+  // CHECK: %[[cast:.*]] = bitcast i32* %{{.*}} to i8*
+  // CHECK-NEXT: %[[cast2:.*]] = 

[PATCH] D62574: Add support for target-configurable address spaces.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I completely lost track of this, but if you'd like to rebase it, I can try to 
make time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62574/new/

https://reviews.llvm.org/D62574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62574: Add support for target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I've been away from LLVM development for a while, and sadly this patch has 
languished a bit.

I don't think there were any strong objections to its inclusion, though. If 
there is still interest in reviewing this, I could try to rebase the patch (or 
something resembling it) onto the latest main and see if it still works.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62574/new/

https://reviews.llvm.org/D62574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62574: Add support for target-configurable address spaces.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Sorry for never actually reviewing this.  I have no objection to taking a 
refactor that implements the Embedded C address-space overlap rules even if we 
don't have an in-tree target that uses it.  I'll try to find time to review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62574/new/

https://reviews.llvm.org/D62574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62574: Add support for target-configurable address spaces.

[Danila Malyutin via Phabricator via cfe-commits]

danilaml added a comment.

So is missing an in-tree user (like AMDGPU) the only thing that prevents from 
merging this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62574/new/

https://reviews.llvm.org/D62574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62574: Add support for target-configurable address spaces.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D62574#2249817 , @ebevhan wrote:

> In D62574#2242471 , @Anastasia wrote:
>
>> The only thing is that it would be good to test the new target setting logic 
>> somehow... do you have any ideas in mind? We could think of creating a dummy 
>> target for that or adding a dummy setting in existing targets - maybe SPIR 
>> could be a candidate. We have done something similar in the past if you look 
>> at `FakeAddrSpaceMap` in `LangOpts`.
>
> Perhaps we could add a configuration to AMDGPU? That has address spaces.
>
> I'm not a big fan of adding an option just for testing.

Ok yes if that can be done it would be better. However, it might need extra 
involvement from AMD side to make sure at least they would agree on maintaining 
this.




Comment at: clang/include/clang/AST/ASTContext.h:2612
+
+  /// Returns true if an explicit cast from address space A to B is legal.
+  /// Explicit conversion between address spaces is permitted if the address

ebevhan wrote:
> Anastasia wrote:
> > Here we should say that this is an extension or enhancement of embedded C 
> > rules that clang implements.
> > 
> > Technically for OpenCL we could refactor to use this functionality as we 
> > don't support such explicit casts on disjoint address spaces. But then this 
> > would not be necessarily a target setting.
> I'm still a bit on the fence about what Embedded-C really stipulates. I don't 
> think it's against the spec to simply disallow disjoint conversion 
> altogether, but it's only necessary to keep Clang's current implementation 
> working.
Technically Clang's policy is to implement documented and standardized 
behavior. So the question is not necessarily about its usefulness, but about 
adhering to good practices. 

I think it is ok to deviate from the standard to make the behavior more helpful 
in some cases but we should aim at documenting such changes. This will help the 
developers in the future if they need to fix bugs or build something on top.



Comment at: clang/lib/AST/ASTContext.cpp:10959
+  // Otherwise, ask the target.
+  return Target->isAddressSpaceSupersetOf(A, B);
+}

ebevhan wrote:
> Anastasia wrote:
> > I guess we should add a similar check here as below?
> > 
> >  
> > ```
> > if (isTargetAddressSpace(From) || isTargetAddressSpace(To) ||
> >   From == LangAS::Default || To == LangAS::Default)
> > ```
> Is it not useful for targets to be able to express relations of LangASes and 
> target ASes?
> 
> The method below must be guarded because otherwise all casts between LangASes 
> would be legal.
I am not sure I follow your comment.

> Is it not useful for targets to be able to express relations of LangASes and 
> target ASes?

yes that's why I was thinking we should add a check to make sure we only ask 
target for target ASes...

However, I am not sure what we should do if there is a mix of target and 
language AS. I guess this should be governed by the target hooks?



Comment at: clang/lib/Sema/SemaOverload.cpp:3235
   //  - in non-top levels it is not a valid conversion.
+  // FIXME: This should probably be using isExplicitAddrSpaceConversionLegal,
+  // but we don't know if this is an implicit or explicit conversion.

ebevhan wrote:
> Anastasia wrote:
> > Sorry if this has been discussed previously, do you refer to the first or 
> > the second case and is there any failing test case?
> It refers to the first case of "valid to convert to addr space that is a 
> superset in all cases". Technically, it could be permitted even if the addr 
> space is not a superset, if this is an explicit cast. But we don't know that. 
> We only know if it's a c-style cast, because those are always 'explicit'.
> 
> I don't have a test case, unfortunately. I just made this observation as I 
> was redoing all of the overlap/superspace checks. It might not even be a 
> problem.
Ok, we could update to:
`
 This should probably be using isExplicitAddrSpaceConversionLegal -> The first 
conversion should probably be using isExplicitAddrSpaceConversionLegal too.`

If you already have a failing test case we could create a bug to track this 
better.



Comment at: clang/lib/Sema/SemaOverload.cpp:5289
 
+  // FIXME: hasAddressSpace is wrong; this check will be skipped if FromType is
+  // not qualified with an address space, but if there's no implicit conversion

ebevhan wrote:
> Anastasia wrote:
> > Do you have a failing test case, if so feel free to create a bug?
> Unsure how I'd make one. I suspect this can't be triggered in OpenCL++, 
> because you can't really have LangAS::Default on FromType there, can you? It 
> would always be some AS.
> 
> Doing it in another way would require a target that has configurable ASes, 
> which doesn't exist yet. Also, it would require methods qualified 

[PATCH] D62574: Add support for target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:10963
+bool
+ASTContext::isExplicitAddrSpaceConversionLegal(LangAS From, LangAS To) const {
+  // If From and To overlap, the cast is legal.

ebevhan wrote:
> Anastasia wrote:
> > Btw I assume that explicit cast can't reject what is not rejected by 
> > implicit cast?
> > 
> > I am not sure if we need to enforce or document this somehow considering 
> > that we provide full configurability now?
> It shouldn't do that, no. I don't think there's any way to guarantee this, 
> though.
> 
> I could add something to the target methods about it.
Wait, no. This is already guaranteed, because the method here in ASTContext 
will check for overlap first.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62574/new/

https://reviews.llvm.org/D62574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62574: Add support for target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D62574#2242471 , @Anastasia wrote:

> The only thing is that it would be good to test the new target setting logic 
> somehow... do you have any ideas in mind? We could think of creating a dummy 
> target for that or adding a dummy setting in existing targets - maybe SPIR 
> could be a candidate. We have done something similar in the past if you look 
> at `FakeAddrSpaceMap` in `LangOpts`.

Perhaps we could add a configuration to AMDGPU? That has address spaces.

I'm not a big fan of adding an option just for testing.




Comment at: clang/include/clang/AST/ASTContext.h:2612
+
+  /// Returns true if an explicit cast from address space A to B is legal.
+  /// Explicit conversion between address spaces is permitted if the address

Anastasia wrote:
> Here we should say that this is an extension or enhancement of embedded C 
> rules that clang implements.
> 
> Technically for OpenCL we could refactor to use this functionality as we 
> don't support such explicit casts on disjoint address spaces. But then this 
> would not be necessarily a target setting.
I'm still a bit on the fence about what Embedded-C really stipulates. I don't 
think it's against the spec to simply disallow disjoint conversion altogether, 
but it's only necessary to keep Clang's current implementation working.



Comment at: clang/lib/AST/ASTContext.cpp:10959
+  // Otherwise, ask the target.
+  return Target->isAddressSpaceSupersetOf(A, B);
+}

Anastasia wrote:
> I guess we should add a similar check here as below?
> 
>  
> ```
> if (isTargetAddressSpace(From) || isTargetAddressSpace(To) ||
>   From == LangAS::Default || To == LangAS::Default)
> ```
Is it not useful for targets to be able to express relations of LangASes and 
target ASes?

The method below must be guarded because otherwise all casts between LangASes 
would be legal.



Comment at: clang/lib/AST/ASTContext.cpp:10963
+bool
+ASTContext::isExplicitAddrSpaceConversionLegal(LangAS From, LangAS To) const {
+  // If From and To overlap, the cast is legal.

Anastasia wrote:
> Btw I assume that explicit cast can't reject what is not rejected by implicit 
> cast?
> 
> I am not sure if we need to enforce or document this somehow considering that 
> we provide full configurability now?
It shouldn't do that, no. I don't think there's any way to guarantee this, 
though.

I could add something to the target methods about it.



Comment at: clang/lib/Sema/SemaCast.cpp:2423
   auto DestPointeeType = DestPtrType->getPointeeType();
-  if (!DestPointeeType.isAddressSpaceOverlapping(SrcPointeeType)) {
+  if (!Self.Context.isExplicitAddrSpaceConversionLegal(
+SrcPointeeType.getQualifiers(), DestPointeeType.getQualifiers())) {

Anastasia wrote:
> Btw just to point our that overlapping used to be a commutative operation so 
> you could swap arguments and still get the same answer but for 
> `isExplicitAddrSpaceConversionLegal` is not the same I assume?
Correct, isExplicitAddrSpaceConversionLegal doesn't have to be commutative.



Comment at: clang/lib/Sema/SemaOverload.cpp:3235
   //  - in non-top levels it is not a valid conversion.
+  // FIXME: This should probably be using isExplicitAddrSpaceConversionLegal,
+  // but we don't know if this is an implicit or explicit conversion.

Anastasia wrote:
> Sorry if this has been discussed previously, do you refer to the first or the 
> second case and is there any failing test case?
It refers to the first case of "valid to convert to addr space that is a 
superset in all cases". Technically, it could be permitted even if the addr 
space is not a superset, if this is an explicit cast. But we don't know that. 
We only know if it's a c-style cast, because those are always 'explicit'.

I don't have a test case, unfortunately. I just made this observation as I was 
redoing all of the overlap/superspace checks. It might not even be a problem.



Comment at: clang/lib/Sema/SemaOverload.cpp:5289
 
+  // FIXME: hasAddressSpace is wrong; this check will be skipped if FromType is
+  // not qualified with an address space, but if there's no implicit conversion

Anastasia wrote:
> Do you have a failing test case, if so feel free to create a bug?
Unsure how I'd make one. I suspect this can't be triggered in OpenCL++, because 
you can't really have LangAS::Default on FromType there, can you? It would 
always be some AS.

Doing it in another way would require a target that has configurable ASes, 
which doesn't exist yet. Also, it would require methods qualified with target 
ASes, and that doesn't work yet either.



Comment at: clang/test/CodeGenCXX/address-space-cast.cpp:41
+  // CHECK: %[[cast:.*]] = bitcast i32* %{{.*}} to i8*
+  // CHECK-NEXT: %[[cast2:.*]] = addrspacecast i8* 

[PATCH] D62574: Add support for target-configurable address spaces.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.
Herald added a subscriber: bjope.

This change looks good to me in general conceptually as it is completing 
missing logic in clang that let's targets to customize the address space 
behavior!

The change also looks good from the implementation side apart from some small 
details raised in the comments.

The only thing is that it would be good to test the new target setting logic 
somehow... do you have any ideas in mind? We could think of creating a dummy 
target for that or adding a dummy setting in existing targets - maybe SPIR 
could be a candidate. We have done something similar in the past if you look at 
`FakeAddrSpaceMap` in `LangOpts`.




Comment at: clang/include/clang/AST/ASTContext.h:2371
+  /// can be safely used as an object qualified with A.
+  bool compatiblyIncludes(Qualifiers A, Qualifiers B) const {
+return isAddressSpaceSupersetOf(A, B) &&

Perhaps not necessarily related to this change but I feel we should provide an 
explanation of how those functions behave wrt address spaces because terms more 
qualified or less qualified don't really apply there.



Comment at: clang/include/clang/AST/ASTContext.h:2588
 
+  /// Returns true if address space A is a superset of B.
+  /// The subspace/superspace relation governs the validity of implicit

I think we should finally start referring to section 5 of embedded C spec as it 
is not a secret that clang implements exactly this. It is rather a good thing 
to document the implemented behavior.



Comment at: clang/include/clang/AST/ASTContext.h:2612
+
+  /// Returns true if an explicit cast from address space A to B is legal.
+  /// Explicit conversion between address spaces is permitted if the address

Here we should say that this is an extension or enhancement of embedded C rules 
that clang implements.

Technically for OpenCL we could refactor to use this functionality as we don't 
support such explicit casts on disjoint address spaces. But then this would not 
be necessarily a target setting.



Comment at: clang/lib/AST/ASTContext.cpp:10959
+  // Otherwise, ask the target.
+  return Target->isAddressSpaceSupersetOf(A, B);
+}

I guess we should add a similar check here as below?

 
```
if (isTargetAddressSpace(From) || isTargetAddressSpace(To) ||
  From == LangAS::Default || To == LangAS::Default)
```



Comment at: clang/lib/AST/ASTContext.cpp:10963
+bool
+ASTContext::isExplicitAddrSpaceConversionLegal(LangAS From, LangAS To) const {
+  // If From and To overlap, the cast is legal.

Btw I assume that explicit cast can't reject what is not rejected by implicit 
cast?

I am not sure if we need to enforce or document this somehow considering that 
we provide full configurability now?



Comment at: clang/lib/Sema/SemaCast.cpp:2423
   auto DestPointeeType = DestPtrType->getPointeeType();
-  if (!DestPointeeType.isAddressSpaceOverlapping(SrcPointeeType)) {
+  if (!Self.Context.isExplicitAddrSpaceConversionLegal(
+SrcPointeeType.getQualifiers(), DestPointeeType.getQualifiers())) {

Btw just to point our that overlapping used to be a commutative operation so 
you could swap arguments and still get the same answer but for 
`isExplicitAddrSpaceConversionLegal` is not the same I assume?



Comment at: clang/lib/Sema/SemaOverload.cpp:3235
   //  - in non-top levels it is not a valid conversion.
+  // FIXME: This should probably be using isExplicitAddrSpaceConversionLegal,
+  // but we don't know if this is an implicit or explicit conversion.

Sorry if this has been discussed previously, do you refer to the first or the 
second case and is there any failing test case?



Comment at: clang/lib/Sema/SemaOverload.cpp:5289
 
+  // FIXME: hasAddressSpace is wrong; this check will be skipped if FromType is
+  // not qualified with an address space, but if there's no implicit conversion

Do you have a failing test case, if so feel free to create a bug?



Comment at: clang/test/CodeGenCXX/address-space-cast.cpp:41
+  // CHECK: %[[cast:.*]] = bitcast i32* %{{.*}} to i8*
+  // CHECK-NEXT: %[[cast2:.*]] = addrspacecast i8* %[[cast]] to i8 
addrspace(5)*
+  // CHECK-NEXT: store i8 addrspace(5)* %[[cast2]]

I am confused about why this is changed now? adrspacecast can cast a type and 
an address space too i.e. it is implicitly a bitcast too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62574/new/

https://reviews.llvm.org/D62574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62574: Add support for target-configurable address spaces.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D62574#2210503 , @ebevhan wrote:

> I think this works now. It should probably be given a few more reviewers to 
> have a look. Do you have some suggestions, @Anastasia ?

Great! I will look at this again within a week or so. I think it would be good 
to get some feedback from John @rjmccall too?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62574/new/

https://reviews.llvm.org/D62574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62574: Add support for target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I think this works now. It should probably be given a few more reviewers to 
have a look. Do you have some suggestions, @Anastasia ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62574/new/

https://reviews.llvm.org/D62574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62574: Add support for target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 284765.
ebevhan retitled this revision from "Initial draft of target-configurable 
address spaces." to "Add support for target-configurable address spaces.".
ebevhan edited the summary of this revision.
ebevhan added a comment.

Rebased and updated summary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62574/new/

https://reviews.llvm.org/D62574

Files:
  clang-tools-extra/clang-tidy/bugprone/VirtualNearMissCheck.cpp
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/CanonicalType.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaFixItUtils.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/test/CodeGenCXX/address-space-cast.cpp
  clang/test/Sema/address_space_print_macro.c
  clang/test/Sema/address_spaces.c

Index: clang/test/Sema/address_spaces.c
===
--- clang/test/Sema/address_spaces.c
+++ clang/test/Sema/address_spaces.c
@@ -71,7 +71,8 @@
 
 // Clang extension doesn't forbid operations on pointers to different address spaces.
 char* cmp(_AS1 char *x,  _AS2 char *y) {
-  return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('_AS1 char *' and '_AS2 char *') which are pointers to non-overlapping address spaces}}
+  return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('_AS1 char *' and '_AS2 char *') which are pointers to non-overlapping address spaces}} \
+// expected-error{{comparison between  ('_AS1 char *' and '_AS2 char *') which are pointers to non-overlapping address spaces}}
 }
 
 char *sub(_AS1 char *x, _AS2 char *y) {
Index: clang/test/Sema/address_space_print_macro.c
===
--- clang/test/Sema/address_space_print_macro.c
+++ clang/test/Sema/address_space_print_macro.c
@@ -14,7 +14,8 @@
 }
 
 char *cmp(AS1 char *x, AS2 char *y) {
-  return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('AS1 char *' and 'AS2 char *') which are pointers to non-overlapping address spaces}}
+  return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('AS1 char *' and 'AS2 char *') which are pointers to non-overlapping address spaces}} \
+// expected-error{{comparison between  ('AS1 char *' and 'AS2 char *') which are pointers to non-overlapping address spaces}}
 }
 
 __attribute__((address_space(1))) char test_array[10];
Index: clang/test/CodeGenCXX/address-space-cast.cpp
===
--- clang/test/CodeGenCXX/address-space-cast.cpp
+++ clang/test/CodeGenCXX/address-space-cast.cpp
@@ -37,8 +37,9 @@
   // CHECK-NEXT: store i8 addrspace(5)* %[[cast]]
   priv_void_ptr = (__private__ void *)gen_void_ptr;
 
-  // CHECK: %[[cast:.*]] = addrspacecast i32* %{{.*}} to i8 addrspace(5)*
-  // CHECK-NEXT: store i8 addrspace(5)* %[[cast]]
+  // CHECK: %[[cast:.*]] = bitcast i32* %{{.*}} to i8*
+  // CHECK-NEXT: %[[cast2:.*]] = addrspacecast i8* %[[cast]] to i8 addrspace(5)*
+  // CHECK-NEXT: store i8 addrspace(5)* %[[cast2]]
   priv_void_ptr = (__private__ void *)gen_int_ptr;
 
   // CHECK: %[[cast:.*]] = addrspacecast i8* %{{.*}} to i32 addrspace(5)*
@@ -65,8 +66,9 @@
   // CHECK-NEXT: call void @_Z10func_pvoidPU3AS5v(i8 addrspace(5)* %[[cast]])
   func_pvoid((__private__ void *)gen_void_ptr);
 
-  // CHECK: %[[cast:.*]] = addrspacecast i32* %{{.*}} to i8 addrspace(5)*
-  // CHECK-NEXT: call void @_Z10func_pvoidPU3AS5v(i8 addrspace(5)* %[[cast]])
+  // CHECK: %[[cast:.*]] = bitcast i32* %{{.*}} to i8*
+  // CHECK-NEXT: %[[cast2:.*]] = addrspacecast i8* %[[cast]] to i8 addrspace(5)*
+  // CHECK-NEXT: call void @_Z10func_pvoidPU3AS5v(i8 addrspace(5)* %[[cast2]])
   func_pvoid((__private__ void *)gen_int_ptr);
 
   // CHECK: %[[cast:.*]] = addrspacecast i8* %{{.*}} to i32 addrspace(5)*
Index: clang/lib/Sema/SemaTemplateDeduction.cpp
===
--- clang/lib/Sema/SemaTemplateDeduction.cpp
+++ clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -1571,7 +1571,8 @@
   // C++ [temp.deduct.conv]p4:
   //   If the original A is a reference type, A can be more cv-qualified
   //   than the deduced A
-  if (!Arg.getQualifiers().compatiblyIncludes(Param.getQualifiers()))
+  if (!S.Context.compatiblyIncludes(Arg.getQualifiers(),
+