[PATCH] D62574: Initial draft of target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D62574#2183294 , @Anastasia wrote:

> In D62574#2136423 , @ebevhan wrote:
>
>> It seems that D70605  attempted to 
>> ameliorate the issues that I observed (pointer-conversion doing too much), 
>> but it didn't manage to solve the problem fully.
>
> Can you explain what's left to be done?

My cache is a bit flushed on this at the moment but I do believe that if the 
issue described in D83325  is resolved, this 
patch should work.




Comment at: lib/Sema/SemaOverload.cpp:3171
+  // qualification conversion for disjoint address spaces make address space
+  // casts *work*?
   Qualifiers FromQuals = FromType.getQualifiers();

Anastasia wrote:
> ebevhan wrote:
> > Anastasia wrote:
> > > ebevhan wrote:
> > > > Anastasia wrote:
> > > > > Anastasia wrote:
> > > > > > ebevhan wrote:
> > > > > > > Anastasia wrote:
> > > > > > > > I guess if address spaces don't overlap we don't have a valid 
> > > > > > > > qualification conversion. This seems to align with the logic 
> > > > > > > > for cv. My guess is that none of the other conversions will be 
> > > > > > > > valid for non overlapping address spaces and the error will 
> > > > > > > > occur.
> > > > > > > > 
> > > > > > > > I think at this point we might not need to know if it's 
> > > > > > > > implicit or explicit? I believe we might have a separate check 
> > > > > > > > for this somewhere because it works for OpenCL. I don't know 
> > > > > > > > though if it might simplify the flow if we move this logic 
> > > > > > > > rather here.
> > > > > > > > 
> > > > > > > > The cv checks above seem to use `CStyle` flag. I am wondering 
> > > > > > > > if we could use it to detect implicit or explicit. Because we 
> > > > > > > > can only cast address space with C style cast at the moment.  
> > > > > > > > Although after adding `addrspace_cast` operator that will no 
> > > > > > > > longer be the only way.
> > > > > > > > I guess if address spaces don't overlap we don't have a valid 
> > > > > > > > qualification conversion. This seems to align with the logic 
> > > > > > > > for cv. My guess is that none of the other conversions will be 
> > > > > > > > valid for non overlapping address spaces and the error will 
> > > > > > > > occur.
> > > > > > > 
> > > > > > > Right. So the reasoning is that if the address spaces are 
> > > > > > > disjoint according to the overlap rule, then it cannot be 
> > > > > > > considered a qualification conversion.
> > > > > > > 
> > > > > > > But with the new hooks, it's possible for a target to allow 
> > > > > > > explicit conversion even if address spaces do not overlap 
> > > > > > > according to the rules. So even though there is no overlap, such 
> > > > > > > a conversion could still be a qualification conversion if it was 
> > > > > > > explicit (either via a c-style cast or an `addrspace_cast`). This 
> > > > > > > is in fact the default for all targets (see the patch in 
> > > > > > > TargetInfo.h).
> > > > > > > 
> > > > > > > I think I need a refresher on how the casts were meant to work; 
> > > > > > > were both `static_cast` and `reinterpret_cast` supposed to be 
> > > > > > > capable of implicit conversion (say, private -> generic) but only 
> > > > > > > `addrspace_cast` for the other direction (generic -> private)? Or 
> > > > > > > was `reinterpret_cast` supposed to fail in the first case?
> > > > > > Just as a summary:
> > > > > > 
> > > > > > - All casts should allow safe/implicit AS conversions i.e. 
> > > > > > `__private`/`__local`/`__global` -> `__generic` in OpenCL
> > > > > > - All casts except for C-style and `addrspace_cast` should disallow 
> > > > > > unsafe/explicit conversions i.e. generic -> 
> > > > > > `__private`/`__local`/`__global` in OpenCL
> > > > > > - All casts should disallow forbidden conversions with no address 
> > > > > > space overlap i.e. `__constant` <-> any other in OpenCL
> > > > > > 
> > > > > > In OpenCL overlapping logic is only used for explicit i.e. unsafe 
> > > > > > conversion. So it seems we might only need those conversions here 
> > > > > > then? 
> > > > > Did you have time to look into this?
> > > > I still don't really understand why the code checks for overlap here. 
> > > > Removing this check causes one test case to break, 
> > > > CodeGenCXX/address-space-cast.cpp. Specifically, this:
> > > > 
> > > > ```
> > > > #define __private__ __attribute__((address_space(5)))
> > > > 
> > > > void test_cast(char *gen_char_ptr, void *gen_void_ptr, int 
> > > > *gen_int_ptr) {
> > > >   __private__ void *priv_void_ptr = (__private__ void *)gen_char_ptr;
> > > > }
> > > > ```
> > > > 
> > > > It tries to resolve the C-style cast with TryAddressSpaceCast, but 
> > > > fails as the underlying pointee types (`char` and `void`) are 
> > > > different. Then it tries to do it 

[PATCH] D62574: Initial draft of target-configurable address spaces.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.



> This was also only an initial concept. I think that even once all the issues 
> with the patch have been ironed out, it would require a round of wider review 
> since it's a fairly hefty API change.

You don't always need a super polished prototype to justify adding new 
functionality. As soon as your design is clearly explained and agreed by the 
relevant stakeholders it should be good enough to kick off the work. Once you 
add some initial implementation it is easier for the community to chip in if 
this functionality is important enough which I sense might be the case.


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: Initial draft of target-configurable address spaces.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D62574#2136423 , @ebevhan wrote:

> It seems that D70605  attempted to 
> ameliorate the issues that I observed (pointer-conversion doing too much), 
> but it didn't manage to solve the problem fully.

Can you explain what's left to be done?




Comment at: lib/Sema/SemaOverload.cpp:3171
+  // qualification conversion for disjoint address spaces make address space
+  // casts *work*?
   Qualifiers FromQuals = FromType.getQualifiers();

ebevhan wrote:
> Anastasia wrote:
> > ebevhan wrote:
> > > Anastasia wrote:
> > > > Anastasia wrote:
> > > > > ebevhan wrote:
> > > > > > Anastasia wrote:
> > > > > > > I guess if address spaces don't overlap we don't have a valid 
> > > > > > > qualification conversion. This seems to align with the logic for 
> > > > > > > cv. My guess is that none of the other conversions will be valid 
> > > > > > > for non overlapping address spaces and the error will occur.
> > > > > > > 
> > > > > > > I think at this point we might not need to know if it's implicit 
> > > > > > > or explicit? I believe we might have a separate check for this 
> > > > > > > somewhere because it works for OpenCL. I don't know though if it 
> > > > > > > might simplify the flow if we move this logic rather here.
> > > > > > > 
> > > > > > > The cv checks above seem to use `CStyle` flag. I am wondering if 
> > > > > > > we could use it to detect implicit or explicit. Because we can 
> > > > > > > only cast address space with C style cast at the moment.  
> > > > > > > Although after adding `addrspace_cast` operator that will no 
> > > > > > > longer be the only way.
> > > > > > > I guess if address spaces don't overlap we don't have a valid 
> > > > > > > qualification conversion. This seems to align with the logic for 
> > > > > > > cv. My guess is that none of the other conversions will be valid 
> > > > > > > for non overlapping address spaces and the error will occur.
> > > > > > 
> > > > > > Right. So the reasoning is that if the address spaces are disjoint 
> > > > > > according to the overlap rule, then it cannot be considered a 
> > > > > > qualification conversion.
> > > > > > 
> > > > > > But with the new hooks, it's possible for a target to allow 
> > > > > > explicit conversion even if address spaces do not overlap according 
> > > > > > to the rules. So even though there is no overlap, such a conversion 
> > > > > > could still be a qualification conversion if it was explicit 
> > > > > > (either via a c-style cast or an `addrspace_cast`). This is in fact 
> > > > > > the default for all targets (see the patch in TargetInfo.h).
> > > > > > 
> > > > > > I think I need a refresher on how the casts were meant to work; 
> > > > > > were both `static_cast` and `reinterpret_cast` supposed to be 
> > > > > > capable of implicit conversion (say, private -> generic) but only 
> > > > > > `addrspace_cast` for the other direction (generic -> private)? Or 
> > > > > > was `reinterpret_cast` supposed to fail in the first case?
> > > > > Just as a summary:
> > > > > 
> > > > > - All casts should allow safe/implicit AS conversions i.e. 
> > > > > `__private`/`__local`/`__global` -> `__generic` in OpenCL
> > > > > - All casts except for C-style and `addrspace_cast` should disallow 
> > > > > unsafe/explicit conversions i.e. generic -> 
> > > > > `__private`/`__local`/`__global` in OpenCL
> > > > > - All casts should disallow forbidden conversions with no address 
> > > > > space overlap i.e. `__constant` <-> any other in OpenCL
> > > > > 
> > > > > In OpenCL overlapping logic is only used for explicit i.e. unsafe 
> > > > > conversion. So it seems we might only need those conversions here 
> > > > > then? 
> > > > Did you have time to look into this?
> > > I still don't really understand why the code checks for overlap here. 
> > > Removing this check causes one test case to break, 
> > > CodeGenCXX/address-space-cast.cpp. Specifically, this:
> > > 
> > > ```
> > > #define __private__ __attribute__((address_space(5)))
> > > 
> > > void test_cast(char *gen_char_ptr, void *gen_void_ptr, int *gen_int_ptr) {
> > >   __private__ void *priv_void_ptr = (__private__ void *)gen_char_ptr;
> > > }
> > > ```
> > > 
> > > It tries to resolve the C-style cast with TryAddressSpaceCast, but fails 
> > > as the underlying pointee types (`char` and `void`) are different. Then 
> > > it tries to do it as a static_cast instead, but fails to produce an 
> > > `AddressSpaceConversion`; instead, it makes a `BitCast` **as the second 
> > > conversion step** which causes CodeGen to break since the conversion kind 
> > > is wrong (the address spaces don't match).
> > > 
> > > Is address space conversion supposed to be a pointer conversion or a 
> > > qualification conversion? If the second conversion step had emitted an 
> > > AddressSpaceConversion instead of a BitCast, it would have worked. But at 

[PATCH] D62574: Initial draft of target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 276163.
ebevhan added a comment.

Rebased.


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
@@ -1457,7 +1457,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(),
+Param.getQualifiers()))
 return Sema::TDK_NonDeducedMismatch;
 
   // Strip out all extra qualifiers from the argument to figure out the
@@ -3389,7 +3390,7 @@
 
 if 

[PATCH] D62574: Initial draft of target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D62574#2136553 , @danilaml wrote:

> In D62574#2135662 , @ebevhan wrote:
>
> > It's generally not safe to alter address spaces below the top level. C is 
> > just very permissive about it.
>
>
> Isn't that also true for the top-level casts? Unless when it is. And the 
> target should know when it's safe or not. It's just that for non top-level 
> casts there is added condition that casts are noop (i.e. don't change the bit 
> pattern of the pointer). At least that's how I see it.


Yes, I see what you mean. I think that the default assumption in Clang at the 
moment is that it is not safe to do so, hence the current design. Not that 
there are many targets that use address spaces, so perhaps that assumption is 
too conservative for ones that do downstream.


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: Initial draft of target-configurable address spaces.

[Danila Malyutin via Phabricator via cfe-commits]

danilaml added a comment.

In D62574#2135662 , @ebevhan wrote:

> It's generally not safe to alter address spaces below the top level. C is 
> just very permissive about it.


Isn't that also true for the top-level casts? Unless when it is. And the target 
should know when it's safe or not. It's just that for non top-level casts there 
is added condition that casts are noop (i.e. don't change the bit pattern of 
the pointer). At least that's how I see it.

I think that you are write about that patch being targeted at C++ (need to do 
some C tests), but having the same implicit conversion being legal in C and not 
in C++ would be a bit confusing for the users (especially when the target knows 
it's completely safe). Anyway, this is not as important as getting this 
target-configurable AS work done. Without it, I guess the only option for 
downstream works to customize the behavior is to introduce their own 
LangAS-level AS entries, which is definitely not ideal.


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: Initial draft of target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

It seems that D70605  attempted to ameliorate 
the issues that I observed (pointer-conversion doing too much), but it didn't 
manage to solve the problem fully.


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: Initial draft of target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a subscriber: danilaml.
ebevhan added a comment.

In D62574#2133160 , @danilaml wrote:

> What are the remaining roadblocks left before this patch can be merged? I'm 
> interested in having a target-specific way to define the allowed 
> explicit/implicit address space conversions.


This has been on my backburner for various reasons, so I'm not sure what the 
status is any more. I could try rebasing it and seeing where things are at.
I don't believe that the issues I mentioned last have been dealt with, though.

This was also only an initial concept. I think that even once all the issues 
with the patch have been ironed out, it would require a round of wider review 
since it's a fairly hefty API change.

> Also, it appears that currently whether implicit casts between pointers of 
> different AS are allowed is determined by the superset relations only, 
> however https://reviews.llvm.org/D73360 introduced another (openCL-specific) 
> variable into the mix: whether the conversion is a top level or not. IMHO, 
> this should also be configured by the target, although I'm not sure whether 
> it needs a separate hook (isBitcastNoop or similar?) or it needs to check the 
> legality of the implicit casts differently.

That patch only seems to apply to C++, I think? The restriction on 
top-level-only conversion should be correct, at least in C++.
It's generally not safe to alter address spaces below the top level. C is just 
very permissive about it.


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: Initial draft of target-configurable address spaces.

[Danila Malyutin via Phabricator via cfe-commits]

danilaml added a comment.

What are the remaining roadblocks left before this patch can be merged? I'm 
interested in having a target-specific way to define the allowed 
explicit/implicit address space conversions.
Also, it appears that currently whether implicit casts between pointers of 
different AS are allowed is determined by the superset relations only, however 
https://reviews.llvm.org/D73360 introduced another (openCL-specific) variable 
into the mix: whether the conversion is a top level or not. IMHO, this should 
also be configured by the target, although I'm not sure whether it needs a 
separate hook (isBitcastNoop or similar?) or it needs to check the legality of 
the implicit casts differently.


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: Initial draft of target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked 2 inline comments as done.
ebevhan added a comment.

Sorry for the very late response on this. Hope it's not completely off your 
radar.




Comment at: include/clang/AST/ASTContext.h:2598
+  /// Returns true if address space A overlaps with B.
+  bool isAddressSpaceOverlapping(LangAS A, LangAS B) const {
+// A overlaps with B if either is a superset of the other.

Anastasia wrote:
> ebevhan wrote:
> > Anastasia wrote:
> > > ebevhan wrote:
> > > > Anastasia wrote:
> > > > > Is there any advantage of keeping superset concept? Amd if 
> > > > > yes, how do we position the new functionality with explicit cast?
> > > > > 
> > > > > I think I am missing a bit conceptual view... because I think we 
> > > > > originally discussed to switch to implicit/explicit conversion model. 
> > > > > Perhaps there is no reason to do it but I would just like to 
> > > > > understand why? 
> > > > Yes, I know the original discussion was regarding the implicit/explicit 
> > > > model, but I came to realize during the implementation that all that 
> > > > was needed to get the superspace model to work generically with the 
> > > > current behavior was an override on the explicit conversion.
> > > > 
> > > > The implicit/explicit model also has the drawback that it's a bit too 
> > > > expressive. You can express semantics that just don't really make 
> > > > sense, like permitting implicit conversion but not explicit conversion. 
> > > > The superspace model doesn't let you do that, and the additions I've 
> > > > made here still don't: If implicit conversion is allowed, explicit 
> > > > conversion must also be allowed. It just becomes possible to allow 
> > > > explicit conversion for ASes that don't overlap.
> > > > 
> > > > Since the superspace model is what OpenCL and Embedded-C use in their 
> > > > specification, it's probably better to use that anyway.
> > > > The implicit/explicit model also has the drawback that it's a bit too 
> > > > expressive. You can express semantics that just don't really make 
> > > > sense, like permitting implicit conversion but not explicit conversion. 
> > > > The superspace model doesn't let you do that, and the additions I've 
> > > > made here still don't: If implicit conversion is allowed, explicit 
> > > > conversion must also be allowed. It just becomes possible to allow 
> > > > explicit conversion for ASes that don't overlap.
> > > 
> > > Ok, I think we could define the new model something like - explicit 
> > > conversions are automatically allowed for all implicit conversions... 
> > > targets won't have to specify those but only extra comversions that are 
> > > not allowed implicitly. 
> > > 
> > > Just to understand in the current patch when are we supposed to use 
> > > `isAddressSpaceOverlapping` and when 
> > > `isExplicitAddrSpaceConversionLegal`. Can't we just always use 
> > > `isExplicitAddrSpaceConversionLegal`?
> > > 
> > > > 
> > > > Since the superspace model is what OpenCL and Embedded-C use in their 
> > > > specification, it's probably better to use that anyway.
> > > 
> > > I agree the advantage of following spec is really huge. However, Clang is 
> > > already broken for Emdedded C isn't it? Because it allows any explicit 
> > > conversions?
> > > 
> > > As for OpenCL it might be reasonable to provide new documentation if 
> > > needed as soon as the new rules don't invalidate all behavior.
> > > 
> > > 
> > > Ok, I think we could define the new model something like - explicit 
> > > conversions are automatically allowed for all implicit conversions... 
> > > targets won't have to specify those but only extra comversions that are 
> > > not allowed implicitly.
> > 
> > Yes, this is how it's defined. Converting explicitly between two ASes where 
> > either one is a superset of the other is always legal.
> > 
> > > Just to understand in the current patch when are we supposed to use 
> > > isAddressSpaceOverlapping and when isExplicitAddrSpaceConversionLegal. 
> > > Can't we just always use isExplicitAddrSpaceConversionLegal?
> > 
> > I guess the distinction between `isAddressSpaceOverlapping` and 
> > `isExplicitAddrSpaceConversionLegal` is pretty subtle. You would want the 
> > former when you need to know if **implicit conversion A->B or B->A** is 
> > permitted, and the latter when you need to know if **explicit conversion 
> > A->B** is permitted.
> > 
> > Though in most cases, I think the latter is probably the most common.
> > 
> > > I agree the advantage of following spec is really huge. However, Clang is 
> > > already broken for Emdedded C isn't it? Because it allows any explicit 
> > > conversions?
> > 
> > No, the current behavior of permitting all explicit conversions is 
> > according to spec: "A non-null pointer into an address space A can be cast 
> > to a pointer into another address space B, but such a cast is undefined if 
> > the source pointer does not point to a location in B." The addition 

[PATCH] D62574: Initial draft of target-configurable address spaces.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: include/clang/AST/ASTContext.h:2598
+  /// Returns true if address space A overlaps with B.
+  bool isAddressSpaceOverlapping(LangAS A, LangAS B) const {
+// A overlaps with B if either is a superset of the other.

ebevhan wrote:
> Anastasia wrote:
> > ebevhan wrote:
> > > Anastasia wrote:
> > > > Is there any advantage of keeping superset concept? Amd if yes, 
> > > > how do we position the new functionality with explicit cast?
> > > > 
> > > > I think I am missing a bit conceptual view... because I think we 
> > > > originally discussed to switch to implicit/explicit conversion model. 
> > > > Perhaps there is no reason to do it but I would just like to understand 
> > > > why? 
> > > Yes, I know the original discussion was regarding the implicit/explicit 
> > > model, but I came to realize during the implementation that all that was 
> > > needed to get the superspace model to work generically with the current 
> > > behavior was an override on the explicit conversion.
> > > 
> > > The implicit/explicit model also has the drawback that it's a bit too 
> > > expressive. You can express semantics that just don't really make sense, 
> > > like permitting implicit conversion but not explicit conversion. The 
> > > superspace model doesn't let you do that, and the additions I've made 
> > > here still don't: If implicit conversion is allowed, explicit conversion 
> > > must also be allowed. It just becomes possible to allow explicit 
> > > conversion for ASes that don't overlap.
> > > 
> > > Since the superspace model is what OpenCL and Embedded-C use in their 
> > > specification, it's probably better to use that anyway.
> > > The implicit/explicit model also has the drawback that it's a bit too 
> > > expressive. You can express semantics that just don't really make sense, 
> > > like permitting implicit conversion but not explicit conversion. The 
> > > superspace model doesn't let you do that, and the additions I've made 
> > > here still don't: If implicit conversion is allowed, explicit conversion 
> > > must also be allowed. It just becomes possible to allow explicit 
> > > conversion for ASes that don't overlap.
> > 
> > Ok, I think we could define the new model something like - explicit 
> > conversions are automatically allowed for all implicit conversions... 
> > targets won't have to specify those but only extra comversions that are not 
> > allowed implicitly. 
> > 
> > Just to understand in the current patch when are we supposed to use 
> > `isAddressSpaceOverlapping` and when `isExplicitAddrSpaceConversionLegal`. 
> > Can't we just always use `isExplicitAddrSpaceConversionLegal`?
> > 
> > > 
> > > Since the superspace model is what OpenCL and Embedded-C use in their 
> > > specification, it's probably better to use that anyway.
> > 
> > I agree the advantage of following spec is really huge. However, Clang is 
> > already broken for Emdedded C isn't it? Because it allows any explicit 
> > conversions?
> > 
> > As for OpenCL it might be reasonable to provide new documentation if needed 
> > as soon as the new rules don't invalidate all behavior.
> > 
> > 
> > Ok, I think we could define the new model something like - explicit 
> > conversions are automatically allowed for all implicit conversions... 
> > targets won't have to specify those but only extra comversions that are not 
> > allowed implicitly.
> 
> Yes, this is how it's defined. Converting explicitly between two ASes where 
> either one is a superset of the other is always legal.
> 
> > Just to understand in the current patch when are we supposed to use 
> > isAddressSpaceOverlapping and when isExplicitAddrSpaceConversionLegal. 
> > Can't we just always use isExplicitAddrSpaceConversionLegal?
> 
> I guess the distinction between `isAddressSpaceOverlapping` and 
> `isExplicitAddrSpaceConversionLegal` is pretty subtle. You would want the 
> former when you need to know if **implicit conversion A->B or B->A** is 
> permitted, and the latter when you need to know if **explicit conversion 
> A->B** is permitted.
> 
> Though in most cases, I think the latter is probably the most common.
> 
> > I agree the advantage of following spec is really huge. However, Clang is 
> > already broken for Emdedded C isn't it? Because it allows any explicit 
> > conversions?
> 
> No, the current behavior of permitting all explicit conversions is according 
> to spec: "A non-null pointer into an address space A can be cast to a pointer 
> into another address space B, but such a cast is undefined if the source 
> pointer does not point to a location in B." The addition of 
> `isExplicitAddrSpaceConversionLegal` lets targets override this behavior and 
> make such casts non-legal.
I see so we are making the new rules then. I guess it should be fine. However 
we might want to document this better and explain the difference between two 
types of API provided.

[PATCH] D62574: Initial draft of target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked 4 inline comments as done.
ebevhan added a comment.

Sorry for the very late response to this!




Comment at: include/clang/AST/ASTContext.h:2598
+  /// Returns true if address space A overlaps with B.
+  bool isAddressSpaceOverlapping(LangAS A, LangAS B) const {
+// A overlaps with B if either is a superset of the other.

Anastasia wrote:
> ebevhan wrote:
> > Anastasia wrote:
> > > Is there any advantage of keeping superset concept? Amd if yes, 
> > > how do we position the new functionality with explicit cast?
> > > 
> > > I think I am missing a bit conceptual view... because I think we 
> > > originally discussed to switch to implicit/explicit conversion model. 
> > > Perhaps there is no reason to do it but I would just like to understand 
> > > why? 
> > Yes, I know the original discussion was regarding the implicit/explicit 
> > model, but I came to realize during the implementation that all that was 
> > needed to get the superspace model to work generically with the current 
> > behavior was an override on the explicit conversion.
> > 
> > The implicit/explicit model also has the drawback that it's a bit too 
> > expressive. You can express semantics that just don't really make sense, 
> > like permitting implicit conversion but not explicit conversion. The 
> > superspace model doesn't let you do that, and the additions I've made here 
> > still don't: If implicit conversion is allowed, explicit conversion must 
> > also be allowed. It just becomes possible to allow explicit conversion for 
> > ASes that don't overlap.
> > 
> > Since the superspace model is what OpenCL and Embedded-C use in their 
> > specification, it's probably better to use that anyway.
> > The implicit/explicit model also has the drawback that it's a bit too 
> > expressive. You can express semantics that just don't really make sense, 
> > like permitting implicit conversion but not explicit conversion. The 
> > superspace model doesn't let you do that, and the additions I've made here 
> > still don't: If implicit conversion is allowed, explicit conversion must 
> > also be allowed. It just becomes possible to allow explicit conversion for 
> > ASes that don't overlap.
> 
> Ok, I think we could define the new model something like - explicit 
> conversions are automatically allowed for all implicit conversions... targets 
> won't have to specify those but only extra comversions that are not allowed 
> implicitly. 
> 
> Just to understand in the current patch when are we supposed to use 
> `isAddressSpaceOverlapping` and when `isExplicitAddrSpaceConversionLegal`. 
> Can't we just always use `isExplicitAddrSpaceConversionLegal`?
> 
> > 
> > Since the superspace model is what OpenCL and Embedded-C use in their 
> > specification, it's probably better to use that anyway.
> 
> I agree the advantage of following spec is really huge. However, Clang is 
> already broken for Emdedded C isn't it? Because it allows any explicit 
> conversions?
> 
> As for OpenCL it might be reasonable to provide new documentation if needed 
> as soon as the new rules don't invalidate all behavior.
> 
> 
> Ok, I think we could define the new model something like - explicit 
> conversions are automatically allowed for all implicit conversions... targets 
> won't have to specify those but only extra comversions that are not allowed 
> implicitly.

Yes, this is how it's defined. Converting explicitly between two ASes where 
either one is a superset of the other is always legal.

> Just to understand in the current patch when are we supposed to use 
> isAddressSpaceOverlapping and when isExplicitAddrSpaceConversionLegal. Can't 
> we just always use isExplicitAddrSpaceConversionLegal?

I guess the distinction between `isAddressSpaceOverlapping` and 
`isExplicitAddrSpaceConversionLegal` is pretty subtle. You would want the 
former when you need to know if **implicit conversion A->B or B->A** is 
permitted, and the latter when you need to know if **explicit conversion A->B** 
is permitted.

Though in most cases, I think the latter is probably the most common.

> I agree the advantage of following spec is really huge. However, Clang is 
> already broken for Emdedded C isn't it? Because it allows any explicit 
> conversions?

No, the current behavior of permitting all explicit conversions is according to 
spec: "A non-null pointer into an address space A can be cast to a pointer into 
another address space B, but such a cast is undefined if the source pointer 
does not point to a location in B." The addition of 
`isExplicitAddrSpaceConversionLegal` lets targets override this behavior and 
make such casts non-legal.



Comment at: lib/Sema/SemaCast.cpp:2224
+// the cast is explicitly legal as well.
+if (CStyle ? !Self.Context.isExplicitAddrSpaceConversionLegal(SrcQ, DestQ)
+   : !Self.Context.isAddressSpaceSupersetOf(DestQ, SrcQ)) {

Anastasia wrote:
> ebevhan 

[PATCH] D62574: Initial draft of target-configurable address spaces.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: include/clang/AST/ASTContext.h:2598
+  /// Returns true if address space A overlaps with B.
+  bool isAddressSpaceOverlapping(LangAS A, LangAS B) const {
+// A overlaps with B if either is a superset of the other.

ebevhan wrote:
> Anastasia wrote:
> > Is there any advantage of keeping superset concept? Amd if yes, how 
> > do we position the new functionality with explicit cast?
> > 
> > I think I am missing a bit conceptual view... because I think we originally 
> > discussed to switch to implicit/explicit conversion model. Perhaps there is 
> > no reason to do it but I would just like to understand why? 
> Yes, I know the original discussion was regarding the implicit/explicit 
> model, but I came to realize during the implementation that all that was 
> needed to get the superspace model to work generically with the current 
> behavior was an override on the explicit conversion.
> 
> The implicit/explicit model also has the drawback that it's a bit too 
> expressive. You can express semantics that just don't really make sense, like 
> permitting implicit conversion but not explicit conversion. The superspace 
> model doesn't let you do that, and the additions I've made here still don't: 
> If implicit conversion is allowed, explicit conversion must also be allowed. 
> It just becomes possible to allow explicit conversion for ASes that don't 
> overlap.
> 
> Since the superspace model is what OpenCL and Embedded-C use in their 
> specification, it's probably better to use that anyway.
> The implicit/explicit model also has the drawback that it's a bit too 
> expressive. You can express semantics that just don't really make sense, like 
> permitting implicit conversion but not explicit conversion. The superspace 
> model doesn't let you do that, and the additions I've made here still don't: 
> If implicit conversion is allowed, explicit conversion must also be allowed. 
> It just becomes possible to allow explicit conversion for ASes that don't 
> overlap.

Ok, I think we could define the new model something like - explicit conversions 
are automatically allowed for all implicit conversions... targets won't have to 
specify those but only extra comversions that are not allowed implicitly. 

Just to understand in the current patch when are we supposed to use 
`isAddressSpaceOverlapping` and when `isExplicitAddrSpaceConversionLegal`. 
Can't we just always use `isExplicitAddrSpaceConversionLegal`?

> 
> Since the superspace model is what OpenCL and Embedded-C use in their 
> specification, it's probably better to use that anyway.

I agree the advantage of following spec is really huge. However, Clang is 
already broken for Emdedded C isn't it? Because it allows any explicit 
conversions?

As for OpenCL it might be reasonable to provide new documentation if needed as 
soon as the new rules don't invalidate all behavior.





Comment at: lib/Sema/SemaCast.cpp:2224
+// the cast is explicitly legal as well.
+if (CStyle ? !Self.Context.isExplicitAddrSpaceConversionLegal(SrcQ, DestQ)
+   : !Self.Context.isAddressSpaceSupersetOf(DestQ, SrcQ)) {

ebevhan wrote:
> Anastasia wrote:
> > It seems like you are changing the functionality here. Don't we need any 
> > test for this?
> I don't think it's necessarily changing. If we are doing a reinterpret_cast 
> that stems from a c-style cast, we want to check that explicit conversion is 
> allowed. This happens both if either AS overlaps, or if the target permits 
> it. If it's not a C-style cast, then we need to check for subspace 
> correctness instead, as reinterpret_cast can only do 'safe' casts.
> 
> The original code here allows all explicit C-style casts regardless of AS, 
> but now we have to ask the target first.
Ok, but shouldn't we test the new functionality of rejecting C style casts if 
target doesn't permit the conversion?

Also I believe C style cast functionality is being handled in 
TryAddressSpaceCast, and it probably belongs there. In fact you have already 
extended this:

```
if (!DestPtrType->isAddressSpaceOverlapping(*SrcPtrType)) {
msg = diag::err_bad_cxx_cast_addr_space_mismatch;
return TC_Failed;
}
```

Why is this change here needed? What test case does it cover?



Comment at: lib/Sema/SemaCast.cpp:2325
+  // Only conversions between pointers to objects in overlapping
+  // addr spaces are allowed, unless the target explicitly allows it.
 

I would keep the OpenCL part but move it to the end as an example. It often 
helps when there are spec references in the code. 


```
// OpenCL v2.0 s6.5.5 - Generic addr space overlaps with any named one, except 
for constant.

```



Comment at: lib/Sema/SemaExpr.cpp:10616
 const PointerType *LHSPtr = LHSType->getAs();
-if 
(!LHSPtr->isAddressSpaceOverlapping(*RHSType->getAs())) {
+if 

[PATCH] D62574: Initial draft of target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked 3 inline comments as done.
ebevhan added a comment.

In D62574#1552220 , @Anastasia wrote:

> Ok, I think at some point you might want to test extra functionality that 
> doesn't fit into OpenCL model, for example explicit conversion over 
> non-overlapping address spaces? I think for this you might find useful to add 
> the extra flag that switches on extra testing with "fake" setup of address 
> spaces.


That functionality is actually enabled by default to allow for Clang's current 
semantics. Embedded-C allows all explicit conversions by default, and OpenCL in 
Clang allows all explicit conversion to and from target spaces as an extension.

It's rather up to each target to *disable* the default explicit conversion 
behavior if they don't want it.




Comment at: include/clang/AST/ASTContext.h:2598
+  /// Returns true if address space A overlaps with B.
+  bool isAddressSpaceOverlapping(LangAS A, LangAS B) const {
+// A overlaps with B if either is a superset of the other.

Anastasia wrote:
> Is there any advantage of keeping superset concept? Amd if yes, how do 
> we position the new functionality with explicit cast?
> 
> I think I am missing a bit conceptual view... because I think we originally 
> discussed to switch to implicit/explicit conversion model. Perhaps there is 
> no reason to do it but I would just like to understand why? 
Yes, I know the original discussion was regarding the implicit/explicit model, 
but I came to realize during the implementation that all that was needed to get 
the superspace model to work generically with the current behavior was an 
override on the explicit conversion.

The implicit/explicit model also has the drawback that it's a bit too 
expressive. You can express semantics that just don't really make sense, like 
permitting implicit conversion but not explicit conversion. The superspace 
model doesn't let you do that, and the additions I've made here still don't: If 
implicit conversion is allowed, explicit conversion must also be allowed. It 
just becomes possible to allow explicit conversion for ASes that don't overlap.

Since the superspace model is what OpenCL and Embedded-C use in their 
specification, it's probably better to use that anyway.



Comment at: lib/Sema/SemaCast.cpp:2224
+// the cast is explicitly legal as well.
+if (CStyle ? !Self.Context.isExplicitAddrSpaceConversionLegal(SrcQ, DestQ)
+   : !Self.Context.isAddressSpaceSupersetOf(DestQ, SrcQ)) {

Anastasia wrote:
> It seems like you are changing the functionality here. Don't we need any test 
> for this?
I don't think it's necessarily changing. If we are doing a reinterpret_cast 
that stems from a c-style cast, we want to check that explicit conversion is 
allowed. This happens both if either AS overlaps, or if the target permits it. 
If it's not a C-style cast, then we need to check for subspace correctness 
instead, as reinterpret_cast can only do 'safe' casts.

The original code here allows all explicit C-style casts regardless of AS, but 
now we have to ask the target first.



Comment at: lib/Sema/SemaCast.cpp:2319
+
+  Kind = CK_AddressSpaceConversion;
+  return TC_Success;

Anastasia wrote:
> Btw I think this is set in:
> https://reviews.llvm.org/D62299
> 
> Although I don't have any objections to changing to this.
Oh, indeed.


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: Initial draft of target-configurable address spaces.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D62574#1534159 , @ebevhan wrote:

> >> I'll have to see if that's possible without breaking a few more 
> >> interfaces, since you can throw around Qualifiers and check for 
> >> compatibility without an ASTContext today.
> >> 
> >>> I was just thinking about testing the new logic. Should we add something 
> >>> like  `-ffake-address-space-map` 
> >>> (https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) 
> >>> that will force targets to use some implementation of fake address space 
> >>> conversions? It was quite useful in OpenCL before we added concrete 
> >>> targets with address spaces. Alternatively, we can try to use SPIR that 
> >>> is a generic Clang-only target that has address spaces.
> >> 
> >> Well, there are a couple targets which have target address spaces even 
> >> today, I think. AMDGPU should be one, right?
> > 
> > Yes, however I am not sure we will be able to test more than what we test 
> > with OpenCL. Also I am not sure AMD maintainer would be ok. Do you have any 
> > concrete idea of what to test?
>
> Well, since there is a mapping from the OpenCL address spaces to the AMDGPU 
> target spaces, I would presume that if the target spaces were used on their 
> own (via `__attribute__((address_space(1)))` for example) they should behave 
> similarly to their OpenCL counterparts. It wouldn't have to be much, just a 
> couple of type definitions and checks for implicit/explicit cast behavior.
>
> There's also the x86 case I mentioned. I'm not really sure how that feature 
> is used, though.


Ok, I think at some point you might want to test extra functionality that 
doesn't fit into OpenCL model, for example explicit conversion over 
non-overlapping address spaces? I think for this you might find useful to add 
the extra flag that switches on extra testing with "fake" setup of address 
spaces.




Comment at: include/clang/AST/ASTContext.h:2598
+  /// Returns true if address space A overlaps with B.
+  bool isAddressSpaceOverlapping(LangAS A, LangAS B) const {
+// A overlaps with B if either is a superset of the other.

Is there any advantage of keeping superset concept? Amd if yes, how do 
we position the new functionality with explicit cast?

I think I am missing a bit conceptual view... because I think we originally 
discussed to switch to implicit/explicit conversion model. Perhaps there is no 
reason to do it but I would just like to understand why? 



Comment at: lib/Sema/SemaCast.cpp:2224
+// the cast is explicitly legal as well.
+if (CStyle ? !Self.Context.isExplicitAddrSpaceConversionLegal(SrcQ, DestQ)
+   : !Self.Context.isAddressSpaceSupersetOf(DestQ, SrcQ)) {

It seems like you are changing the functionality here. Don't we need any test 
for this?



Comment at: lib/Sema/SemaCast.cpp:2319
+
+  Kind = CK_AddressSpaceConversion;
+  return TC_Success;

Btw I think this is set in:
https://reviews.llvm.org/D62299

Although I don't have any objections to changing to this.



Comment at: lib/Sema/SemaCast.cpp:2334
   // warn even though local -> generic is permitted.
-  if (Self.getLangOpts().OpenCL) {
-const Type *DestPtr, *SrcPtr;

I think you might need to update the comment above to reflect that you are 
generalizing the behavior.  


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: Initial draft of target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 203559.
ebevhan added a comment.

Replaced `compatiblyIncludes` and its dependents with ASTContext accessors 
instead.


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

https://reviews.llvm.org/D62574

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/CanonicalType.h
  include/clang/AST/Type.h
  include/clang/Basic/TargetInfo.h
  lib/AST/ASTContext.cpp
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaExceptionSpec.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaFixItUtils.cpp
  lib/Sema/SemaInit.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  test/Sema/address_space_print_macro.c
  test/Sema/address_spaces.c

Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ 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}}
 }
 
 struct SomeStruct {
Index: test/Sema/address_space_print_macro.c
===
--- test/Sema/address_space_print_macro.c
+++ 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: lib/Sema/SemaTemplateDeduction.cpp
===
--- lib/Sema/SemaTemplateDeduction.cpp
+++ lib/Sema/SemaTemplateDeduction.cpp
@@ -1444,7 +1444,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(),
+Param.getQualifiers()))
 return Sema::TDK_NonDeducedMismatch;
 
   // Strip out all extra qualifiers from the argument to figure out the
@@ -3212,7 +3213,7 @@
 
 if (AQuals == DeducedAQuals) {
   // Qualifiers match; there's nothing to do.
-} else if (!DeducedAQuals.compatiblyIncludes(AQuals)) {
+} else if (!S.Context.compatiblyIncludes(DeducedAQuals, AQuals)) {
   return Failed();
 } else {
   // Qualifiers are compatible, so have the argument type adopt the
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -2394,7 +2394,7 @@
   if (TQs == Qs)
 return T;
 
-  if (Qs.compatiblyIncludes(TQs))
+  if (Context.compatiblyIncludes(Qs, TQs))
 return Context.getQualifiedType(T, Qs);
 
   return Context.getQualifiedType(T.getUnqualifiedType(), Qs);
@@ -2430,8 +2430,8 @@
   const ObjCInterfaceType* LHS = ToObjCPtr->getInterfaceType();
   const ObjCInterfaceType* RHS = FromObjCPtr->getInterfaceType();
   if (getLangOpts().CPlusPlus && LHS && RHS &&
-  !ToObjCPtr->getPointeeType().isAtLeastAsQualifiedAs(
-FromObjCPtr->getPointeeType()))
+  !Context.isAtLeastAsQualifiedAs(ToObjCPtr->getPointeeType(),
+  FromObjCPtr->getPointeeType()))
 return false;
   ConvertedType = BuildSimilarlyQualifiedPointerType(FromObjCPtr,
ToObjCPtr->getPointeeType(),
@@ -2617,7 +2617,7 @@
 
   // Make sure that we have compatible qualifiers.
   FromQuals.setObjCLifetime(Qualifiers::OCL_Autoreleasing);
-  if (!ToQuals.compatiblyIncludes(FromQuals))
+  if (!Context.compatiblyIncludes(ToQuals, FromQuals))
 return false;
 
   // Remove qualifiers from the pointee type we're 

[PATCH] D62574: Initial draft of target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

>> I'll have to see if that's possible without breaking a few more interfaces, 
>> since you can throw around Qualifiers and check for compatibility without an 
>> ASTContext today.
>> 
>>> I was just thinking about testing the new logic. Should we add something 
>>> like  `-ffake-address-space-map` 
>>> (https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) that 
>>> will force targets to use some implementation of fake address space 
>>> conversions? It was quite useful in OpenCL before we added concrete targets 
>>> with address spaces. Alternatively, we can try to use SPIR that is a 
>>> generic Clang-only target that has address spaces.
>> 
>> Well, there are a couple targets which have target address spaces even 
>> today, I think. AMDGPU should be one, right?
> 
> Yes, however I am not sure we will be able to test more than what we test 
> with OpenCL. Also I am not sure AMD maintainer would be ok. Do you have any 
> concrete idea of what to test?

Well, since there is a mapping from the OpenCL address spaces to the AMDGPU 
target spaces, I would presume that if the target spaces were used on their own 
(via `__attribute__((address_space(1)))` for example) they should behave 
similarly to their OpenCL counterparts. It wouldn't have to be much, just a 
couple of type definitions and checks for implicit/explicit cast behavior.

There's also the x86 case I mentioned. I'm not really sure how that feature is 
used, though.


Repository:
  rC Clang

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: Initial draft of target-configurable address spaces.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D62574#1527126 , @ebevhan wrote:

> In D62574#1523835 , @Anastasia wrote:
>
> > > This patch does not address the issue with the accessors
> > >  on Qualifiers (isAddressSpaceSupersetOf, compatiblyIncludes),
> > >  because I don't know how to solve it without breaking a ton of
> > >  rather nice encapsulation. Either, every mention of compatiblyIncludes
> > >  must be replaced with a call to a corresponding ASTContext method,
> > >  Qualifiers must have a handle to ASTContext, or ASTContext must be
> > >  passed to every such call. This revision mentions the issue in a comment:
> > >  https://reviews.llvm.org/D49294
> >
> > I think using ASTContext helper is ok then.
>
>
> Alright. Just to clarify, you mean the first option (using a new accessor on 
> ASTContext and replacing all existing compatiblyIncludes with that)?


Yes, it seems ok to me. Not sure if @rjmccall has an opinion.

> I'll have to see if that's possible without breaking a few more interfaces, 
> since you can throw around Qualifiers and check for compatibility without an 
> ASTContext today.
> 
>> I was just thinking about testing the new logic. Should we add something 
>> like  `-ffake-address-space-map` 
>> (https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) that 
>> will force targets to use some implementation of fake address space 
>> conversions? It was quite useful in OpenCL before we added concrete targets 
>> with address spaces. Alternatively, we can try to use SPIR that is a generic 
>> Clang-only target that has address spaces.
> 
> Well, there are a couple targets which have target address spaces even today, 
> I think. AMDGPU should be one, right?

Yes, however I am not sure we will be able to test more than what we test with 
OpenCL. Also I am not sure AMD maintainer would be ok. Do you have any concrete 
idea of what to test?

> I forgot to mention; this patch also disables two "extensions" that Clang 
> has, namely that subtraction and comparison on pointers of different address 
> spaces is legal regardless of address space compatibility. I'm pretty sure 
> that these extensions only exist because Clang hasn't had support for targets 
> to express address space compatibility until now. According to the docs, x86 
> uses address spaces for certain types of segment access: 
> https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments
> 
> If x86 (or any target that uses target address spaces) wants to keep using 
> this and still keep such pointers implicitly compatible with each other, it 
> will need to configure this in its target hooks.

Ok, it seems logical to me. In OpenCL we don't allow those cases.




Comment at: lib/Sema/SemaOverload.cpp:3171
+  // qualification conversion for disjoint address spaces make address space
+  // casts *work*?
   Qualifiers FromQuals = FromType.getQualifiers();

ebevhan wrote:
> Anastasia wrote:
> > I guess if address spaces don't overlap we don't have a valid qualification 
> > conversion. This seems to align with the logic for cv. My guess is that 
> > none of the other conversions will be valid for non overlapping address 
> > spaces and the error will occur.
> > 
> > I think at this point we might not need to know if it's implicit or 
> > explicit? I believe we might have a separate check for this somewhere 
> > because it works for OpenCL. I don't know though if it might simplify the 
> > flow if we move this logic rather here.
> > 
> > The cv checks above seem to use `CStyle` flag. I am wondering if we could 
> > use it to detect implicit or explicit. Because we can only cast address 
> > space with C style cast at the moment.  Although after adding 
> > `addrspace_cast` operator that will no longer be the only way.
> > I guess if address spaces don't overlap we don't have a valid qualification 
> > conversion. This seems to align with the logic for cv. My guess is that 
> > none of the other conversions will be valid for non overlapping address 
> > spaces and the error will occur.
> 
> Right. So the reasoning is that if the address spaces are disjoint according 
> to the overlap rule, then it cannot be considered a qualification conversion.
> 
> But with the new hooks, it's possible for a target to allow explicit 
> conversion even if address spaces do not overlap according to the rules. So 
> even though there is no overlap, such a conversion could still be a 
> qualification conversion if it was explicit (either via a c-style cast or an 
> `addrspace_cast`). This is in fact the default for all targets (see the patch 
> in TargetInfo.h).
> 
> I think I need a refresher on how the casts were meant to work; were both 
> `static_cast` and `reinterpret_cast` supposed to be capable of implicit 
> conversion (say, private -> generic) but only `addrspace_cast` for the other 
> direction 

[PATCH] D62574: Initial draft of target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D62574#1523835 , @Anastasia wrote:

> > This patch does not address the issue with the accessors
> >  on Qualifiers (isAddressSpaceSupersetOf, compatiblyIncludes),
> >  because I don't know how to solve it without breaking a ton of
> >  rather nice encapsulation. Either, every mention of compatiblyIncludes
> >  must be replaced with a call to a corresponding ASTContext method,
> >  Qualifiers must have a handle to ASTContext, or ASTContext must be
> >  passed to every such call. This revision mentions the issue in a comment:
> >  https://reviews.llvm.org/D49294
>
> I think using ASTContext helper is ok then.


Alright. Just to clarify, you mean the first option (using a new accessor on 
ASTContext and replacing all existing compatiblyIncludes with that)?

I'll have to see if that's possible without breaking a few more interfaces, 
since you can throw around Qualifiers and check for compatibility without an 
ASTContext today.

> I was just thinking about testing the new logic. Should we add something like 
>  `-ffake-address-space-map` 
> (https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) that 
> will force targets to use some implementation of fake address space 
> conversions? It was quite useful in OpenCL before we added concrete targets 
> with address spaces. Alternatively, we can try to use SPIR that is a generic 
> Clang-only target that has address spaces.

Well, there are a couple targets which have target address spaces even today, I 
think. AMDGPU should be one, right?

I forgot to mention; this patch also disables two "extensions" that Clang has, 
namely that subtraction and comparison on pointers of different address spaces 
is legal regardless of address space compatibility. I'm pretty sure that these 
extensions only exist because Clang hasn't had support for targets to express 
address space compatibility until now. According to the docs, x86 uses address 
spaces for certain types of segment access: 
https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments

If x86 (or any target that uses target address spaces) wants to keep using this 
and still keep such pointers implicitly compatible with each other, it will 
need to configure this in its target hooks.




Comment at: lib/Sema/SemaOverload.cpp:3171
+  // qualification conversion for disjoint address spaces make address space
+  // casts *work*?
   Qualifiers FromQuals = FromType.getQualifiers();

Anastasia wrote:
> I guess if address spaces don't overlap we don't have a valid qualification 
> conversion. This seems to align with the logic for cv. My guess is that none 
> of the other conversions will be valid for non overlapping address spaces and 
> the error will occur.
> 
> I think at this point we might not need to know if it's implicit or explicit? 
> I believe we might have a separate check for this somewhere because it works 
> for OpenCL. I don't know though if it might simplify the flow if we move this 
> logic rather here.
> 
> The cv checks above seem to use `CStyle` flag. I am wondering if we could use 
> it to detect implicit or explicit. Because we can only cast address space 
> with C style cast at the moment.  Although after adding `addrspace_cast` 
> operator that will no longer be the only way.
> I guess if address spaces don't overlap we don't have a valid qualification 
> conversion. This seems to align with the logic for cv. My guess is that none 
> of the other conversions will be valid for non overlapping address spaces and 
> the error will occur.

Right. So the reasoning is that if the address spaces are disjoint according to 
the overlap rule, then it cannot be considered a qualification conversion.

But with the new hooks, it's possible for a target to allow explicit conversion 
even if address spaces do not overlap according to the rules. So even though 
there is no overlap, such a conversion could still be a qualification 
conversion if it was explicit (either via a c-style cast or an 
`addrspace_cast`). This is in fact the default for all targets (see the patch 
in TargetInfo.h).

I think I need a refresher on how the casts were meant to work; were both 
`static_cast` and `reinterpret_cast` supposed to be capable of implicit 
conversion (say, private -> generic) but only `addrspace_cast` for the other 
direction (generic -> private)? Or was `reinterpret_cast` supposed to fail in 
the first case?



Comment at: lib/Sema/SemaOverload.cpp:5150
 
+  // 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:
> Agree! We need to check that address spaces are not identical and then 
> validity!
> 
> But I guess it's ok for C++ because we can't add addr space qualifier to 
> implicit object parameter yet?
> 
> It's 

[PATCH] D62574: Initial draft of target-configurable address spaces.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

> This patch does not address the issue with the accessors
>  on Qualifiers (isAddressSpaceSupersetOf, compatiblyIncludes),
>  because I don't know how to solve it without breaking a ton of
>  rather nice encapsulation. Either, every mention of compatiblyIncludes
>  must be replaced with a call to a corresponding ASTContext method,
>  Qualifiers must have a handle to ASTContext, or ASTContext must be
>  passed to every such call. This revision mentions the issue in a comment:
>  https://reviews.llvm.org/D49294

I think using ASTContext helper is ok then.

I was just thinking about testing the new logic. Should we add something like  
`-ffake-address-space-map` 
(https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) that 
will force targets to use some implementation of fake address space 
conversions? It was quite useful in OpenCL before we added concrete targets 
with address spaces. Alternatively, we can try to use SPIR that is a generic 
Clang-only target that has address spaces.




Comment at: lib/Sema/SemaOverload.cpp:3171
+  // qualification conversion for disjoint address spaces make address space
+  // casts *work*?
   Qualifiers FromQuals = FromType.getQualifiers();

I guess if address spaces don't overlap we don't have a valid qualification 
conversion. This seems to align with the logic for cv. My guess is that none of 
the other conversions will be valid for non overlapping address spaces and the 
error will occur.

I think at this point we might not need to know if it's implicit or explicit? I 
believe we might have a separate check for this somewhere because it works for 
OpenCL. I don't know though if it might simplify the flow if we move this logic 
rather here.

The cv checks above seem to use `CStyle` flag. I am wondering if we could use 
it to detect implicit or explicit. Because we can only cast address space with 
C style cast at the moment.  Although after adding `addrspace_cast` operator 
that will no longer be the only way.



Comment at: lib/Sema/SemaOverload.cpp:5150
 
+  // 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

Agree! We need to check that address spaces are not identical and then validity!

But I guess it's ok for C++ because we can't add addr space qualifier to 
implicit object parameter yet?

It's broken for OpenCL though!


Repository:
  rC Clang

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: Initial draft of target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan created this revision.
ebevhan added reviewers: Anastasia, rjmccall.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is an initial draft of the support for target-configurable
address spaces.

Original RFC: http://lists.llvm.org/pipermail/cfe-dev/2019-March/061541.html

After thinking about Anastasia's input on the RFC regarding
the superspace/subspace model, I decided to go a different
route with the design of the interface. Instead of having
accessors for implicit casts and explicit casts, I kept the
subspace accessor and added an accessor for overriding
the behavior of explicit casts only.

This lets us keep the overlap model and support the default
Embedded-C functionality while still letting targets
configure their behavior reasonably.

This patch does not address the issue with the accessors
on Qualifiers (isAddressSpaceSupersetOf, compatiblyIncludes),
because I don't know how to solve it without breaking a ton of
rather nice encapsulation. Either, every mention of compatiblyIncludes
must be replaced with a call to a corresponding ASTContext method,
Qualifiers must have a handle to ASTContext, or ASTContext must be
passed to every such call. This revision mentions the issue in a comment:
https://reviews.llvm.org/D49294


Repository:
  rC Clang

https://reviews.llvm.org/D62574

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  include/clang/Basic/TargetInfo.h
  lib/AST/ASTContext.cpp
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaInit.cpp
  lib/Sema/SemaOverload.cpp
  test/Sema/address_space_print_macro.c
  test/Sema/address_spaces.c

Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ 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}}
 }
 
 struct SomeStruct {
Index: test/Sema/address_space_print_macro.c
===
--- test/Sema/address_space_print_macro.c
+++ 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: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -3161,12 +3161,17 @@
   = PreviousToQualsIncludeConst && ToQuals.hasConst();
   }
 
-  // Allows address space promotion by language rules implemented in
-  // Type::Qualifiers::isAddressSpaceSupersetOf.
+  // FIXME: This should *really* be testing implicit conversions with
+  // 'isAddressSpaceSupersetOf' and explicit conversions with
+  // 'isExplicitAddrSpaceConversionLegal', but IsQualificationConversion isn't
+  // aware of whether the conversion is implicit or explicit. Add that?
+  //
+  // Also, I don't really understand why we do this. How does preventing
+  // qualification conversion for disjoint address spaces make address space
+  // casts *work*?
   Qualifiers FromQuals = FromType.getQualifiers();
   Qualifiers ToQuals = ToType.getQualifiers();
-  if (!ToQuals.isAddressSpaceSupersetOf(FromQuals) &&
-  !FromQuals.isAddressSpaceSupersetOf(ToQuals)) {
+  if (!Context.isAddressSpaceOverlapping(ToQuals, FromQuals)) {
 return false;
   }
 
@@ -5142,10 +5147,14 @@
 return ICS;
   }
 
+  // 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
+  // from Default to ImplicitParamType's AS, that's an error.
   if (FromTypeCanon.getQualifiers().hasAddressSpace()) {
 Qualifiers QualsImplicitParamType = ImplicitParamType.getQualifiers();
 Qualifiers QualsFromType =