[PATCH] D56066: [OpenCL] Address space for default class members

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/Sema/SemaInit.cpp:4539
+  if (InitCategory.isPRValue() || InitCategory.isXValue())
+    T1Quals.removeAddressSpace();
+
----------------
Anastasia wrote:
> rjmccall wrote:
> > ebevhan wrote:
> > > rjmccall wrote:
> > > > rjmccall wrote:
> > > > > I can understand why a pr-value wouldn't have an address space, but an x-value's address space is surely important.
> > > > No, wait, I think this is more deeply wrong.  We're initializing a reference to type `cv1 T1`, and if we're initializing it with a temporary, we're dropping the address space from `cv1`?  That's definitely wrong.
> > > > 
> > > > If we're starting with a pr-value, reference-initialization normally has to start by initializing a temporary.  (The exception is it's a class type with a conversion operator to a reference type; C++ is abysmally complicated.  Let's ignore this for a moment.)  Temporaries are always in the default address space (the address space of local variables) because the language (neither OpenCL nor Embedded C) does not give us any facility for allocating temporary memory anywhere else.  So reference-initialization from a pr-value creates a temporary in the default address space and then attempts to initialize the destination reference type with that temporary.  That initialization should fail if there's no conversion from the default address space to the destination address space.  For example, consider initializing a `const int __global &` from a pr-value: this should clearly not succeed, because we have no way of putting a temporary in the global address space.  That reference can only be initialized with a gl-value that's already in the `__global` address space.
> > > > 
> > > > On the other hand, we can successfully initialize a `const int &` with a gl-value of type `const int __global` not by direct reference initialization but by loading from the operand and then materializing the result into a new temporary.
> > > > 
> > > > I think what this means is:
> > > > 
> > > > - We need to be doing this checking as if pr-values were in `__private`.  This includes both (1) expressions that were originally pr-values and (2) expressions that have been converted to pr-values due to a failure to perform direct reference initialization.
> > > > 
> > > > - We need to make sure that reference compatibility correctly prevents references from being bound to gl-values in incompatible address spaces.
> > > > On the other hand, we can successfully initialize a `const int &` with a gl-value of type `const int __global` not by direct reference initialization but by loading from the operand and then materializing the result into a new temporary.
> > > 
> > > Is this the right thing to do? It means that initializing such a reference would incur a cost under the hood that might be unnoticed/unwanted by the user. 
> > Hmm.  I was going to say that it's consistent with the normal behavior of C++, but looking at the actual rule, it's more complicated than that: C++ will only do this if the types are not reference-related or if the gl-value is a bit-field.  So this would only be allowed if the reference type was e.g. `const long &`.  It's a strange rule, but it's straightforward to stay consistent with it.
> > 
> > I think we will eventually find ourselves needing a special-case rule for copy-initializing and copy-assigning trivially-copyable types, but we're not there yet.
> I need some help to understand what's the right way to fix this problem.
> 
> As far as I gathered from this particular example. The issues is caused by the following statement of **addrspace-of-this.cl** line 47:
>   C c3 = c1 + c2;
> 
> If I remove this new `if` statement in **SemaInit.cpp** and disable some asserts about the incorrect address space cast the AST that Clang is trying to create is as follows:
> 
> 
> ```
> | | |-DeclStmt 0x9d6e70 <line:47:3, col:17>
> | | | `-VarDecl 0x9d6b28 <col:3, col:15> col:5 c3 'C' cinit
> | | |   `-ExprWithCleanups 0x9d6e58 <col:10, col:15> 'C'
> | | |     `-CXXConstructExpr 0x9d6e20 <col:10, col:15> 'C' 'void (__generic C &&) __generic' elidable
> | | |       `-MaterializeTemporaryExpr 0x9d6e08 <col:10, col:15> '__generic C' xvalue
> | | |         `-ImplicitCastExpr 0x9d6df0 <col:10, col:15> '__generic C' <AddressSpaceConversion>
> | | |           `-CXXOperatorCallExpr 0x9d6c90 <col:10, col:15> 'C'
> | | |             |-ImplicitCastExpr 0x9d6c78 <col:13> 'C (*)(const __generic C &) __generic' <FunctionToPointerDecay>
> | | |             | `-DeclRefExpr 0x9d6bf8 <col:13> 'C (const __generic C &) __generic' lvalue CXXMethod 0x9d4da0 'operator+' 'C (const __generic C &) __generic'
> | | |             |-ImplicitCastExpr 0x9d6be0 <col:10> '__generic C' lvalue <AddressSpaceConversion>
> | | |             | `-DeclRefExpr 0x9d6b88 <col:10> 'C' lvalue Var 0x9d6830 'c1' 'C'
> | | |             `-ImplicitCastExpr 0x9d6bc8 <col:15> 'const __generic C' lvalue <AddressSpaceConversion>
> | | |               `-DeclRefExpr 0x9d6ba8 <col:15> 'C' lvalue Var 0x9d6928 'c2' 'C'
> ```
> 
> I feel that this AST (kind of) makes sense? The result of `operator+` returns class `C` object but as it's directly used to copy construct `c3` it seems logical to cast it to `__generic C`.
> 
> Before references with address spaces existed we only had to cast pointers to different address spaces... but now in this particular case of initializing a reference, casting an address space of non-pointer type seem to make sense?
> 
> Do you think the AST is correct and I should just change the address space related asserts in casting and teach CodeGen how to generate `addrspacecast` for such reference initialization? Or do you think the AST has to be created differently?
> 
> As a side note: I wish we would have some 'special' AST node to obtain a reference to an object just like we are taking an address of an object to become a pointer, that we could construct implicitly. It would make our life easier for such cases.
This mostly looks right, but I think the `MaterializeTemporaryExpr` ought to create the temporary in the private address space, and then there should be an ICE to convert that to the generic address space, rather than the other way around.


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

https://reviews.llvm.org/D56066