Page MenuHomePhabricator

[OpenCL] Address space for default class members
AbandonedPublic

Authored by mikael on Dec 13 2018, 7:30 AM.

Details

Summary
  • Implicity and explicity defaulted class members
  • Resolved the FIXMEs in addrspace-of-this.cl

Diff Detail

Event Timeline

mikael created this revision.Dec 13 2018, 7:30 AM
mikael marked an inline comment as done.Dec 13 2018, 7:41 AM
mikael added inline comments.
lib/Sema/SemaInit.cpp
4539

This is probably not the correct way to solve this. But it fixes the issues that i get.

The code that triggers this issue is found in test/CodeGenOpenCLCXX/addrspace-of-this.cl:47

C c3 = c1 + c2;

So the original (InitializedEntity) Entity.getType() looks like:

RValueReferenceType 0x717470 '__generic class C &&'
`-QualType 0x717458 '__generic class C' __generic
  `-RecordType 0x717160 'class C'
    `-CXXRecord 0x7170d0 'C'

It might be so that we actually want to qualify the RValueReferenceType rather than the RecordType here.

rjmccall added inline comments.Dec 13 2018, 12:52 PM
lib/AST/ASTContext.cpp
2781

You're trying to handle a method qualifier, not a type a functions that are themselves in some non-standard address space, right? The method qualifier should already be part of Proto->getExtProtoInfo(), so if there's an address space qualifier out here, something is very wrong.

Anastasia added inline comments.Dec 17 2018, 5:20 AM
lib/AST/ASTContext.cpp
2781

As far as I understand the new design, we have an address space qualifier on a method and as a part of the function prototype too. Are you saying that we need to make sure the prototype has an address space too?

rjmccall added inline comments.Dec 17 2018, 12:07 PM
lib/AST/ASTContext.cpp
2781

The address-space this qualifiers are a part of FunctionProtoType that's totally independent from the storage of top-level qualifiers that's part of QualType. Orig.getAddressSpace() is asking about the top-level qualifiers, not the this qualifiers. Generally, function types should not have top-level qualifiers at all (although of course a member pointer can have both top-level qualifiers and this qualifiers, with the former meaning something completely different from the latter).

ebevhan added inline comments.
lib/AST/ASTContext.cpp
2781

I suspect the reason there has to be qualifiers on the function type itself is because of the getType mistake I mentioned in the previous patch. If the this address space qualifiers aren't also on the function type, then GetThisType will not produce the correctly qualified type.

If the getType in GetThisType is changed to getTypeQualifiers, then the function type should not need any qualifiers.

Anastasia added inline comments.Mon, Dec 24, 10:19 AM
lib/AST/ASTContext.cpp
2781

I fixed the issues reported here and in https://reviews.llvm.org/D54862 on the following review: https://reviews.llvm.org/D56066.

Anastasia requested changes to this revision.Mon, Dec 24, 10:23 AM

@mikael can you please close this review, because I opened another one to continue this: https://reviews.llvm.org/D56066.

Thanks for your work!

This revision now requires changes to proceed.Mon, Dec 24, 10:23 AM
mikael abandoned this revision.Tue, Jan 8, 11:38 PM