Anastasia marked an inline comment as done. Anastasia added inline comments.
================ Comment at: lib/Sema/SemaInit.cpp:4539 + if (InitCategory.isPRValue() || InitCategory.isXValue()) + T1Quals.removeAddressSpace(); + ---------------- 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56066/new/ https://reviews.llvm.org/D56066 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
