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

Mon Jun 3 11:31:48 PDT 2019

Anastasia added a comment.

In D62574#1527126 <https://reviews.llvm.org/D62574#1527126>, @ebevhan wrote:

> In D62574#1523835 <https://reviews.llvm.org/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 (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? 

================
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
----------------
ebevhan wrote:
> 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 broken for OpenCL though!
> > But I guess it's ok for C++ because we can't add addr space qualifier to implicit object parameter yet?
> 
> True, but that will become possible eventually.
> 
> I'm not sure if this is currently an issue with OpenCL as it is either. It's only a problem if FromType is not qualified, but is that possible? Even the 'notational' non-AS-qualification in OpenCL is still a qualification under the hood (either private or generic). Or maybe I'm misunderstanding how it works.
Yes, ideally we deduce address spaces if they are not specified explicitly. However I am not sure this is currently the case everywhere and whether we can have weird interplay with templates logic. One case I have in mind is function return type that is kept with `Default` address space but it shouldn't  ever occur with the logic here.

So perhaps theoretically it's not broken for OpenCL. :)

Repository:
  rC Clang

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

https://reviews.llvm.org/D62574