Page MenuHomePhabricator

[OpenCL] Address space for default class members
ClosedPublic

Authored by Anastasia on Dec 24 2018, 10:17 AM.

Details

Summary

Fixed several issues reported in https://reviews.llvm.org/D55656 and https://reviews.llvm.org/D54862 - mainly removed addr space from the QualType of the method itself.

This should make all implicitly and explicity defaulted special class members work now!

Since Mikael is on holiday and I have no permission to upload to his review, I have to create a new one.

Diff Detail

Event Timeline

Anastasia created this revision.Dec 24 2018, 10:17 AM
rjmccall added inline comments.Dec 31 2018, 12:48 PM
lib/CodeGen/CGCall.cpp
78

Would you mind making a patch to rename this (and the method on FunctionProtoType) to something like getMethodQualifiers? It can be a follow-up instead of blocking this.

lib/Sema/SemaDeclCXX.cpp
12648

You're implicitly dropping the address space qualifier here by going back to ClassType.

lib/Sema/SemaInit.cpp
4539

I can understand why a pr-value wouldn't have an address space, but an x-value's address space is surely important.

Anastasia updated this revision to Diff 179899.Jan 2 2019, 11:14 AM

Addressed review comments.

Anastasia marked an inline comment as done.Jan 2 2019, 11:16 AM
Anastasia added inline comments.
lib/CodeGen/CGCall.cpp
78

Sure. I will prepare a separate patch for this.

rjmccall added inline comments.Jan 2 2019, 10:05 PM
lib/Sema/SemaInit.cpp
4539

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.
ebevhan added inline comments.Jan 7 2019, 1:40 AM
lib/Sema/SemaInit.cpp
4539

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.

rjmccall added inline comments.Jan 7 2019, 12:07 PM
lib/Sema/SemaInit.cpp
4539

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.

Anastasia marked an inline comment as done.Jan 9 2019, 10:09 AM
Anastasia added inline comments.
lib/Sema/SemaInit.cpp
4539

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.

rjmccall added inline comments.Jan 9 2019, 1:49 PM
lib/Sema/SemaInit.cpp
4539

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.

Modified conversion sequence to perform address space conversion after temporary materialization conversion.

Removed FIXME that this patch fixes.

rjmccall added inline comments.Jan 10 2019, 11:43 AM
lib/AST/Expr.cpp
1618

What's this about? I would expect that the VK of this and its sub-expression are always the same.

lib/CodeGen/CGExprAgg.cpp
813

If there's a Dest, you might need to reverse-convert its address space, right?

lib/Sema/SemaInit.cpp
4686

Okay. The existing behavior seems a little strange, but, well, it's existing behavior.

Anastasia updated this revision to Diff 181256.Jan 11 2019, 5:53 AM
  • Set correctly value kind of address space conversion after materializing the temporary
  • Removed changes in various places including CodeGen for address space conversion that became unnecessary after the AST is generated correctly.
  • Updated tests to check for the right IR and also not to use hard-coded registers.
Anastasia marked 6 inline comments as done.Jan 11 2019, 5:56 AM
Anastasia added inline comments.
lib/CodeGen/CGExprAgg.cpp
813

After I fixed the AST (by setting VK correctly on ICE) we are no longer hitting this path. So I removed the unnecessary changes.

rjmccall accepted this revision.Jan 14 2019, 12:21 AM

Great, LGTM.

This revision is now accepted and ready to land.Jan 14 2019, 12:21 AM
This revision was automatically updated to reflect the committed changes.
Anastasia marked 2 inline comments as done.Jan 16 2019, 9:16 AM
Anastasia added inline comments.
lib/CodeGen/CGCall.cpp
78