Page MenuHomePhabricator

[OpenCL] Add generic AS to 'this' pointer
ClosedPublic

Authored by mikael on Nov 23 2018, 7:05 AM.

Diff Detail

Repository
rC Clang

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mikael marked an inline comment as done.Nov 27 2018, 7:27 AM
mikael added inline comments.
lib/CodeGen/CGCall.cpp
4052

Thanks for the input! It seems like the name performAddrSpaceConversion does not exist in the code-base though.

rjmccall added inline comments.Nov 27 2018, 8:14 AM
lib/CodeGen/CGCall.cpp
4052

Sorry, performAddrSpaceCast.

Anastasia added inline comments.Nov 27 2018, 11:54 AM
lib/CodeGen/CGCall.cpp
81

Thanks! Yes, I believe some template-based approach could make this a lot more usable. I would be really interested to learn more about your ideas. I would also quite like to see this properly in the spec. I will follow up with you in a separate thread.

mikael updated this revision to Diff 175646.Nov 28 2018, 2:59 AM
rjmccall added inline comments.Nov 28 2018, 8:36 AM
lib/AST/DeclCXX.cpp
2191–2192

I feel like the right design now is for getTypeQualifiers() to return a Qualifiers and not just a mask.

That will also help identify other places that need to be updated to support address-space qualification.

lib/CodeGen/CGCall.cpp
72–73

Please update the comment to say "ignoring method CVR qualification" instead.

Also, maybe we should just eliminate this and use CXXMethodDecl::getThisType. If we want to preserve the micro-optimization of sharing CGFunctionInfos for method types that only differ by CV qualification, maybe we should add a parameter to getThisType to not add ABI-compatible qualification.

lib/CodeGen/CGClass.cpp
2020

This is adding an extra level of pointer, I think.

2023

...I'm not sure when getLangASFromTargetAS was added, but it shouldn't exist; you need to pass down the language address space of This, which in general will come from the pointer type. And then the AS != LangAS::Default check should just be comparing the constructor's address space to that address space, not specifically LangAS::Default.

Same comments apply to the code in commonEmitCXXMemberOrOperatorCall.

mikael updated this revision to Diff 176063.Nov 30 2018, 2:24 AM
mikael marked an inline comment as done.Nov 30 2018, 2:33 AM
mikael added inline comments.
lib/Sema/SemaOverload.cpp
5261

Note that this change allowed me to revert some of the CG changes.

rjmccall added inline comments.Nov 30 2018, 9:59 AM
include/clang/AST/DeclCXX.h
2193

The address-space qualifier should get stored in the same way on the FunctionProtoType as the other method qualifiers (at least as far as the public interface of FPT goes). It's not the same thing as an ordinary type qualifier on the type, and trying to represent it that way will lead to a lot of problems downstream.

You'll probably also have to do some extra work in SemaType to make sure that the attribute gets applied in the right place.

You'll need to mess around with the representation of FPT to make this efficient, because we don't want to make every instance of FPT have to pay to store a full Qualifiers. I'd do it like this:

  • Go ahead and add a full Qualifiers in FunctionProtoType::ExtProtoInfo; this is never stored permanently, so the cost is negligible.
  • There are currently four bits for "qualifiers" in FunctionTypeBits. The only qualifiers that are actually important to store inline like this are const and volatile, but since we have four bits, let's store CVR here and use the fourth bit to indicate that there are more qualifiers stored in trailing storage.
  • When there are non-CVR qualifiers present, ASTContext::getFunctionTypeInternal should allocate additional trailing storage for a Qualifiers object that can store the rest of the qualifiers.
  • getTypeQuals() can check for the bit and load the qualifiers if present, or else just return the CVR qualifiers stored inline.
mikael updated this revision to Diff 176596.Dec 4 2018, 4:10 AM
Anastasia added inline comments.Dec 4 2018, 4:46 AM
lib/Sema/SemaType.cpp
4829

Can this be moved into deduceOpenCLImplicitAddrSpace?

mikael marked an inline comment as done.Dec 4 2018, 5:02 AM
mikael added inline comments.
lib/Sema/SemaType.cpp
4829

deduceOpenCLImplicitAddrSpace is only wrapping a type with an address space. I also need to make sure the FunctionProtoType gets generated with the address space.

Anastasia added inline comments.Dec 4 2018, 6:01 AM
lib/Sema/SemaType.cpp
4829

FunctionProtoType is a type too. Does it go into deduceOpenCLImplicitAddrSpace when creating this?

I have a lot of comments about various things that need to be fixed, but I just want to take a moment to say that this is a tremendously impressive patch: given only some very high-level directions, you've done a great job figuring out what you needed to do to carry that out and make it work. Really well done.

lib/AST/ASTDumper.cpp
209

We must have some existing code to print an address space.

lib/AST/ItaniumMangle.cpp
1507

You can overload based on the address space, right? I think it needs to be mangled.

lib/AST/TypePrinter.cpp
803–804

You should be printing all the qualifiers here, including address spaces. If you want to suppress an implicit OpenCL address-space qualifier, that seems reasonable.

lib/CodeGen/CGDebugInfo.cpp
2619–2620

I think this is just GetThisType again. Maybe there should be a version of that takes a record and an FPT; you could make it a static method on CXXMethodDecl. At any rate, I don't think you shouldn't be dropping the address space.

lib/Index/USRGeneration.cpp
275

Paging @akyrtzi here. The address-space qualifier should be part of the USR but I don't think if there's a defined schema for that.

lib/Parse/ParseCXXInlineMethods.cpp
419

This should take the full qualifiers. You can see how it ends up getting reassembled into the this type in the CXXThisScopeRAII ctor, and that type definitely needs to be address-space-qualified.

lib/Sema/SemaCodeComplete.cpp
1031

Hmm. This is actually going to break code completion for these methods, I think: IIUC in OpenCL every pointer and l-value is address-space-qualified, so only considering the CVRU qualifiers means that there will always be "extra" qualifiers on the pointer and code completion will think it doesn't match.

...I know code completion isn't your top priority, but this is probably easy enough to fix.

3741

This seems to be intentionally dropping non-CVR qualifiers, and I don't know why you ever would want to do that.

lib/Sema/SemaDeclCXX.cpp
12000

This should definitely not be dropping address spaces. You'll need to make a second patch to handle special members correctly, so you don't need to add a test for this yet, but you might as well get the logic right right now.

12367

Same thing as above.

lib/Sema/SemaOverload.cpp
1146

This is an algorithm that I think you need to get right and which shouldn't drop address spaces.

2828

This is fair to punt to a follow-up patch, but you *should* make that patch.

5071

You should be using the full qualifiers here.

lib/Sema/SemaTemplateDeduction.cpp
4662

Again, this is dropping non-CVR qualifiers for no particular reason.

mikael marked an inline comment as done and an inline comment as not done.Dec 5 2018, 5:32 AM

Thanks for the feedback, I'll work on fixing the issues!

lib/Sema/SemaOverload.cpp
1146

But with this patch we can only have __generic address space for methods. So if I try to implement something here I can't really test it? Or am I missing something?

Anastasia added inline comments.Dec 5 2018, 6:19 AM
lib/AST/ItaniumMangle.cpp
1507

Does this refer to our earlier discussion https://reviews.llvm.org/D54862#inline-484509

We don't have a way to qualify methods with an address space yet? I was going to send an RFC to cfe-dev for this but if you think it would be ok to go ahead with an implementation, I am happy with it. Either way would it be better to do this in a separate patch?

rjmccall added inline comments.Dec 5 2018, 7:44 AM
lib/AST/ItaniumMangle.cpp
1507

I'm fine with delaying implementation on these two issues until a later patch since, as you say, they can't be tested well until we support arbitrary address-space qualifiers. Please at least leave FIXMEs for them.

Anastasia added inline comments.Dec 5 2018, 7:49 AM
lib/Sema/SemaOverload.cpp
1146

Since we didn't add address spaces to the method qualifiers yet, it seems logical to ignore them.

Although we could already say, since we would like to add such overloading anyway, it would be logical to return true for the address space qualifiers mismatch just like it's done for CV quals in line 1154. However, I can't think of any way we could test this indeed.

mikael updated this revision to Diff 176831.Dec 5 2018, 7:58 AM

I uploaded a new patch for most of the comments. I did leave some things out since they need clarification.

mikael marked an inline comment as done.Dec 5 2018, 8:00 AM
mikael added inline comments.
lib/AST/ItaniumMangle.cpp
1507

I actually did update this, since we can test the mangling of __generic in the test.

akyrtzi added inline comments.Dec 5 2018, 8:17 AM
lib/Index/USRGeneration.cpp
275

If I understand correctly from other comments you can't add special mangling and USR generation yet and intend to add FIXME for doing mangling support later ? If yes, could you also add FIXME here and re-visit ?

rjmccall added inline comments.Dec 5 2018, 9:18 PM
lib/AST/ItaniumMangle.cpp
1507

Great, thanks. Looks good.

lib/AST/Type.cpp
2959

The indentation here is messed up.

You seem to be mixing up "fast qualifiers" with "CVR qualifiers" in a few places. That's currently correct, in the sense that the fast qualifiers are currently defined to be the CVR qualifiers, but the point of the distinction is that we might want to change that (and we have in fact explored that in the past). I think you should make FunctionTypeBits only claim to store the fast qualifiers, which mostly just means updating a few field / accessor names and changing things like the getCVRQualifiers() call right above this to be getFastQualifiers().

If there are places where it's convenient to assume that the CVR qualifiers are exactly the fast qualifiers, it's okay to static_assert that, but you should make sure to static_assert it in each place you assume it.

lib/CodeGen/CGDebugInfo.cpp
2621

Why is this getMostRecentCXXRecordDecl instead of getClass?

lib/CodeGen/CGDeclCXX.cpp
32

What causes OpenCL C++ declarations to go down this path?

lib/Index/USRGeneration.cpp
275

While you're here, can you described the right way for us to extend USR generation here to potentially add an address space?

lib/Parse/ParseDecl.cpp
6166

I think fromCVRUQualifiers is probably the right function, and if that doesn't exist you should add it.

lib/Sema/SemaExprCXX.cpp
1112

I don't think there's a good reason for this to be dropping non-CVR qualifiers. If there is, it really needs a comment.

lib/Sema/TreeTransform.h
5276

I think you need to turn ThisTypeQuals into a Qualifiers here.

mikael marked 3 inline comments as done.Dec 6 2018, 12:02 AM
mikael added inline comments.
lib/AST/Type.cpp
2959

I'll change it, but I do have a question related to this:

Can we make any assumptions with the "fast qualifiers"? I'd like it if we can assume that they fit in 3 bits. Otherwise I think I also need an assertion here.

lib/CodeGen/CGDebugInfo.cpp
2621

Your initial comment suggested to send in a record rather than a type. But I see now that it may make more sense to use the type directly instead. Will update it.

lib/CodeGen/CGDeclCXX.cpp
32

It goes there when we defined an object in __local address space. From the spec "The constructors of a objects in local memory should only be initialized once". This is the same case as with C++ 11 static local variables.

mikael updated this revision to Diff 176943.Dec 6 2018, 2:43 AM
  • Added FIXME's.
  • Fixed the latest comments.
mikael marked an inline comment as done and an inline comment as not done.Dec 6 2018, 2:53 AM
mikael added inline comments.
lib/AST/Type.cpp
2959

I solved it like this in the end:

  • I defined the FastTypeQuals to get the size of Qualifiers::FastWidth to make it dependent on possible changes to the fast flags.
  • Then I put a static_assertion to guard the hastConst(), hasVolatile() and hasRestrict() methods.
rjmccall accepted this revision.Dec 6 2018, 10:12 AM

LGTM, but you can probably undo one of my requests. :)

lib/AST/Type.cpp
2959

Great, looks good.

lib/CodeGen/CGDebugInfo.cpp
2621

Oh, I actually didn't even think about the fact that the getClass()) returns a Type* instead of a CXXRecordDecl. Using the record decl is probably better, sorry.

This revision is now accepted and ready to land.Dec 6 2018, 10:12 AM
akyrtzi added inline comments.Dec 6 2018, 11:50 AM
lib/Index/USRGeneration.cpp
275

I'm not familiar with the mechanism, is address space supposed to be identified via a single integer ? If so, then I think adding '-' character plus the integer as character for the address space, immediately after the type qualifiers, should be a way to go.

mikael updated this revision to Diff 177477.Dec 10 2018, 2:41 AM
mikael marked an inline comment as not done.

I rebased on Friday, and noticed that I broke two tests:

Failing Tests (2):

Clang :: CodeGenOpenCLCXX/template-address-spaces.cl
Clang :: SemaOpenCLCXX/address-space-templates.cl

This upload contains a few extra fixes.

mikael marked 4 inline comments as done.Dec 10 2018, 2:56 AM
mikael added inline comments.
lib/Sema/SemaDecl.cpp
3196

When merging the class function and the file context function the address space was lost here.

lib/Sema/SemaType.cpp
4830

The first issue was that if a function is declared outside of a class this code did not deduce it. Now it does.

lib/Sema/TreeTransform.h
4277–4279

Another place where the address space was removed.

test/SemaOpenCLCXX/address-space-templates.cl
7–9

This was the remaining issue that I have not solved yet. It looked like this issue was not so trivial so I think it makes sense to postpone it.

Ka-Ka added a subscriber: Ka-Ka.Dec 11 2018, 1:15 AM
Anastasia added inline comments.Dec 11 2018, 2:53 AM
lib/Sema/SemaType.cpp
3931

From the state you can use getDeclarator() that then call a method getContext().

5170

Rather than passing DeclContext you can get it from the Declarator using getContext() method.

lib/Sema/TreeTransform.h
4277–4279

Why OpenCL in the comment, since the code seems to be rather generic.

Also not sure this comments adds anything that can't be understood from the code you are adding.

test/SemaOpenCLCXX/address-space-templates.cl
7–9

Do you know why it appears twice?

mikael marked an inline comment as done.Dec 11 2018, 2:59 AM
mikael added inline comments.
test/SemaOpenCLCXX/address-space-templates.cl
7–9

I believe the main issue is that we have some code that does the following in SemaTemplateInstantiateDecl.cpp:

OldTL.getAs<FunctionProtoTypeLoc>()

But the implementation of getAs returns nullptr if you have local modifiers. So since this code will always return nullptr we end up taking a different paths.

mikael marked an inline comment as done.Dec 11 2018, 4:00 AM
mikael added inline comments.
lib/Sema/SemaType.cpp
5170

Declarator::getContext() returns an enumeration DeclaratorContext which is not enough in this case.

Anastasia accepted this revision.Dec 12 2018, 2:41 AM

LGTM! Apart from small improvement that can done before committing if it works at all. :)

lib/Sema/SemaType.cpp
5170

Ok, I was wondering if you can use scope clang::NestedNameSpecifier::SpecifierKind to check if something is a method instead of passing DC down.

This revision was automatically updated to reflect the committed changes.

Seems like my this commit broke: http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-buildserver/builds/33176

But since I don't really know what anything about lldb, I probably won't be able to fix it fast enough.

/lldb-buildbot/buildServerSlave/lldb-android-buildserver/llvm/tools/lldb/source/Symbol/ClangASTContext.cpp:2208:24: error: no viable overloaded '='
  proto_info.TypeQuals = type_quals;
  ~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
/lldb-buildbot/buildServerSlave/lldb-android-buildserver/llvm/tools/clang/include/clang/AST/Type.h:141:7: note: candidate function (the implicit copy assignment operator) not viable: no known conversion from 'unsigned int' to 'const clang::Qualifiers' for 1st argument
class Qualifiers {
      ^
/lldb-buildbot/buildServerSlave/lldb-android-buildserver/llvm/tools/clang/include/clang/AST/Type.h:141:7: note: candidate function (the implicit move assignment operator) not viable: no known conversion from 'unsigned int' to 'clang::Qualifiers' for 1st argument
1 error generated.

I think it can be easily solved by Qualifiers::fromFastMask(type_quals); also updating the variable name since it changed.

But I know nothing about lldb, so I better revert this patch for now.

riccibruno added inline comments.
include/clang/AST/Type.h
3692

You can use FunctionType::FunctionTypeExtraBitfields
to store the qualifiers. It was designed to store extra data.
No need to add yet another trailing object.

Seems like my this commit broke: http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-buildserver/builds/33176

But since I don't really know what anything about lldb, I probably won't be able to fix it fast enough.

/lldb-buildbot/buildServerSlave/lldb-android-buildserver/llvm/tools/lldb/source/Symbol/ClangASTContext.cpp:2208:24: error: no viable overloaded '='
  proto_info.TypeQuals = type_quals;
  ~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
/lldb-buildbot/buildServerSlave/lldb-android-buildserver/llvm/tools/clang/include/clang/AST/Type.h:141:7: note: candidate function (the implicit copy assignment operator) not viable: no known conversion from 'unsigned int' to 'const clang::Qualifiers' for 1st argument
class Qualifiers {
      ^
/lldb-buildbot/buildServerSlave/lldb-android-buildserver/llvm/tools/clang/include/clang/AST/Type.h:141:7: note: candidate function (the implicit move assignment operator) not viable: no known conversion from 'unsigned int' to 'clang::Qualifiers' for 1st argument
1 error generated.

I think it can be easily solved by Qualifiers::fromFastMask(type_quals); also updating the variable name since it changed.

I think that's exactly what you need to do, and you'd be welcome to commit that instead of reverting this.

lib/Index/USRGeneration.cpp
275

Okay. Some of them are integers; others are identified in the language as more of a keyword and so there's no stable mapping from an integer value. Should we just come up with some simple schema for that following - that's unambiguous with a digit?

ebevhan added inline comments.
lib/CodeGen/CGCall.cpp
77

I'm a bit late to the party, but shouldn't this be MD->getTypeQualifiers().getAddressSpace()?

rjmccall added inline comments.Dec 17 2018, 12:10 PM
lib/CodeGen/CGCall.cpp
77

Yes, you're right, it should.

I'm also a bit confused about the semantics that this patch is applying to function types. It mostly seems to concern the extra trailing Qualifiers on CXXMethodDecl to store the addrspace quals, but in some places (SemaType:4842, SemaDecl:3198) it seems to be applying the address space to the function type itself, which certainly seems like something else to me. A function with an address space qualified type would (to me, at least) be a function *located* in that address space, not one qualified to take a this from that address space.

I'm also a bit confused about the semantics that this patch is applying to function types. It mostly seems to concern the extra trailing Qualifiers on CXXMethodDecl to store the addrspace quals, but in some places (SemaType:4842, SemaDecl:3198) it seems to be applying the address space to the function type itself, which certainly seems like something else to me. A function with an address space qualified type would (to me, at least) be a function *located* in that address space, not one qualified to take a this from that address space.

Yeah, there may be some confusion about that, and unfortunately it's easy to miss in the review.

But yes, I agree that address-space qualifiers are in principle one of the few qualifiers that make sense to be able to have (in a normal way) on function types.