Page MenuHomePhabricator

[OpenCL] Allow address spaces as method qualifiers
Needs ReviewPublic

Authored by Anastasia on Dec 18 2018, 12:39 PM.

Details

Reviewers
rjmccall
Summary

This patch allows qualifying methods with address spaces and extends some overloading rules to use those qualifiers.

The main use case is to prevent conversions to generic/default address space where it's requested by the programmer. More details can be found in:
http://lists.llvm.org/pipermail/cfe-dev/2018-December/060470.html

This patch doesn't enable the feature for C++ yet but it can easily be generalized if the overall flow is right.

Diff Detail

Event Timeline

Anastasia created this revision.Dec 18 2018, 12:39 PM

Removed FIXME that has been addressed

Anastasia marked an inline comment as done.Dec 18 2018, 12:47 PM
Anastasia added inline comments.
lib/Sema/SemaOverload.cpp
2854–2855

I am still missing something here.

You're gating on OpenCL C++ in several places in this patch, but I don't think you need to. This extension should be available to anyone using address spaces and C++.

lib/Parse/ParseDecl.cpp
6155

This is enforcing a restriction that users write const __private, which seems unreasonable. It looks like ParseTypeQualifierList takes a flag saying not to parse attributes; try adding a new option to that enum allowing address-space attributes.

Collecting the attributes on the type-qualifiers DeclSpec rather than adding them as function attributes seems correct.

lib/Sema/SemaOverload.cpp
1187

If you leave OldQuals and NewQuals as Qualifiers and then just remove restrict from both sets, I think you don't need a special check for address spaces here.

6706

I would expect this to fail in TryObjectArgumentInitialization above and not to need a special case here.

9309

This at least needs a comment explaining the rule you're trying to implement.

Anastasia marked an inline comment as done.

Address review comments.

lib/Parse/ParseDecl.cpp
6155

Do you mean ParseTypeQualifierListOpt? That already parses the address spaces unconditionally. The problem is however that we are using local DeclSpec - DS variable here during the function qualifiers parsing because the DeclSpec member of the Declarator corresponds to the return type as far as I understand it. Therefore I am propagating missing address space attributes from the local DS variable into FnAttrs to be used in the function type info.

With current patch both of the following two forms:

struct C {
  void foo() __local const;
};

and

struct C {
  void foo() const __local;
};

would be parsed correctly generating identical IR

declare void @_ZNU3AS3K1C3fooEv(%struct.C addrspace(3)*)

You're gating on OpenCL C++ in several places in this patch, but I don't think you need to. This extension should be available to anyone using address spaces and C++.

Ok, I am happy to generalize it to C++. There are some slight differences in the address space attribute representation for C/C++ as I am now figuring out. So I would prefer to put a separate patch if it's ok with you.

Anastasia marked 3 inline comments as done and an inline comment as not done.Wed, Dec 19, 10:04 AM
rjmccall added inline comments.Wed, Dec 19, 10:44 AM
lib/Parse/ParseDecl.cpp
6155

Oh, I see, sorry. Why filter on attributes at all, then? We should *only* be parsing qualifier attributes in that list.

I actually think it would be reasonable to change DeclaratorChunk::FunctionTypeInfo to just store a DeclSpec for all the qualifiers; we're already duplicating an unfortunate amount of the logic from DeclSpec, like remembering SourceLocations for all the qualifiers. You'll have to store it out-of-line, but we already store e.g. parameters out of line, so the machinery for allocating and destroying it already exists. Just make sure we don't actually allocate anything in the common case where there aren't any qualifiers (i.e. for C and non-member C++ functions).

Also, I suspect that the use of CXXThisScopeRAII below this needs to be updated to pull *all* the qualifiers out of DS. Maybe Sema should have a function for that.

lib/Sema/SemaOverload.cpp
2854–2855

Well, at least the failure here is just to fall into the generic diagnostic.

Getting this diagnostic right probably requires some minor work to the diagnostics engine. If you look at err_init_conversion_failed, which is (I think) the diagnostic that's always being used here, it matches every possible CVR mask so that it can pretty-print them. This is already a problem because the input is actually a CVRU mask! A better option would be to teach DiagnosticEngine how to store and format a Qualifiers value, and then you can just stream the original Qualifiers into the diagnostic here.

But that's obviously work for a separate patch.

9309

Okay, thanks for the clarification. I think this should just be part of CompareImplicitConversionSequence, right? It seems to me that you should prefer e.g. int __private * -> int __private * over int __generic * as a normal argument conversion as well.

Also, can this be written in terms of isAddressSpaceSupersetOf? I don't remember how LangAS::Default works in OpenCL C++ enough to understand how it fits in here.

Anastasia marked an inline comment as done.Thu, Dec 20, 12:39 PM
Anastasia added inline comments.
lib/Parse/ParseDecl.cpp
6155

I have uploaded a separate patch for this:
https://reviews.llvm.org/D55948

I think I can't avoid storing DeclSpec for non-memeber functions in C++ because we still use them to give diagnostics about the qualifiers. For example:

test.cpp:1:12: error: non-member function cannot have 'const' qualifier
void foo() const;

As for CXXThisScopeRAII, we seem to be adding other qualifiers to the QualType directly (while building it), so there seems to be no function to collect them into Qualifiers. I can, however, add code to collect address space qualifiers here. I guess we don't have any other missing qualifiers to collect?

rjmccall added inline comments.Thu, Dec 20, 2:47 PM
lib/Parse/ParseDecl.cpp
6155

I think you can probably avoid allocating and storing a DeclSpec if there are no qualifiers, which should be easy to test, and then the absence of a DeclSpec should allow Sema to quickly bail out of the check for illegal qualifiers on a non-member function type.

I guess we don't have any other missing qualifiers to collect?

Not that are legal to apply to this.

Anastasia marked an inline comment as done.Fri, Dec 28, 6:50 AM
Anastasia added inline comments.
lib/Sema/SemaOverload.cpp
9309

That's correct we should implement the same logic for the arguments too. I will create a helper function or do you think we should just call CompareImplicitConversionSequence on the method type too?

I think isAddressSpaceSupersetOf can't be used here because it determines compatibility of address spaces. However, the logic we are expressing is for the address space preference instead.

rjmccall added inline comments.Mon, Dec 31, 1:13 PM
lib/Sema/SemaOverload.cpp
9309

If I understand correctly, we already call CompareImplicitConversionSequence on the object-argument conversion above, as part of the for (unsigned ArgIdx = StartArg; ArgIdx < NumArgs; ++ArgIdx) loop.

I think the address-space ordering rule can be reasonably based on compatibility. In fact, I think it already is in our implementation. The right rule is basically that (1) an exact match is better than a conversion and (2) a conversion to a subspace is better than a conversion to a superspace. You can think of this as modifying the "proper subset" rule of [over.ics.rank]p3.2.5, which in implementation terms means Qualifiers::isMoreQualifiedThan, which already ends up using isAddressSpaceSupersetOf. So if this ranking isn't working right, it's probably because we're incorrectly fast-pathing based on CVR qualifiers somewhere, and in fact I can see a pretty suspicious check like that in CompareQualificationConversions.

Anastasia marked an inline comment as done.Wed, Jan 2, 3:37 AM
Anastasia added inline comments.
lib/Sema/SemaOverload.cpp
2854–2855

I created a patch for this in https://reviews.llvm.org/D56198.

brunodf added inline comments.
lib/Sema/SemaType.cpp
4874–4875

I follow all of the above (from the point "a class member function has an address space"), but then I take issue with this line (already from Mikael).

You look at the declaration's attributes, to derive the ASIdx relating to the method's this argument. You mark the relevant attributes as invalid, to prevent them from being considered in "processTypeAttrs" after the switch that we break below. The ASIdx is stored in the qualifiers EPI to go to the FunctionProtoType (this will affect getThisType). This all seems fine.

But then this line adds the address space qualification to T, the type of the declared function itself. This seems unnecessary, and conceptually wrong: while the function may have an address space in syntax, this address space applies to the this argument, not to the function object (and a pointer to the function is not a pointer to this address space etc.). In short, I think this line should be removed.

Rebased on top of recent related changes and addressed remaining review comments.

This now depends on: https://reviews.llvm.org/D56735

Anastasia marked 3 inline comments as done.Tue, Jan 15, 11:07 AM
Anastasia added inline comments.
lib/Sema/SemaOverload.cpp
9309

I created a separate review to fix this: https://reviews.llvm.org/D56735

lib/Sema/SemaType.cpp
4874–4875

I think I fixed it in: https://reviews.llvm.org/D56066

Okay, so is this ready to re-review independently?

Anastasia marked an inline comment as done.Wed, Jan 16, 2:36 AM

Okay, so is this ready to re-review independently?

Yes, please. It should be fine to review on its own. Thanks!

Anastasia updated this revision to Diff 182012.Wed, Jan 16, 4:37 AM

Fixed wording on the comment.

rjmccall added inline comments.Wed, Jan 16, 12:41 PM
lib/Parse/ParseDecl.cpp
6170

Does this not need to diagnose redundant qualifiers? Why is this path required in addition to the path in SemaType, anyway?

lib/Sema/SemaType.cpp
3930

I'd just drop this else, but if you want to include it, please use braces for consistency with the rest of the if-chain.

Anastasia updated this revision to Diff 182281.Thu, Jan 17, 6:56 AM
Anastasia marked an inline comment as done.
  • Removed else case in DiagnoseMultipleAddrSpaceAttributes
  • Added extra test to check correctness of addr space for 'this' when statically used in a class scope.
Anastasia marked an inline comment as done.Thu, Jan 17, 7:01 AM
Anastasia added inline comments.
lib/Parse/ParseDecl.cpp
6170

We discussed earlier to collect addr space qualifiers for completeness: https://reviews.llvm.org/D55850#inline-495037

Without this change the following doesn't work for addr spaces correctly:

auto fGlob() __global -> decltype(this);

I added a test that check this now in test/SemaOpenCLCXX/address-space-of-this-class-scope.cl.

Here we only collect the quals to be applied to 'this'. If there are multiple address space specified on a method it will be diagnosed in SemaType.cpp. I think we probably don't need to give the same diagnostic twice?

Anastasia updated this revision to Diff 182282.Thu, Jan 17, 7:05 AM
  • Fixed typo in FIXME
rjmccall added inline comments.Thu, Jan 17, 5:47 PM
lib/Parse/ParseDecl.cpp
6170

Okay, if this is already diagnosed, it isn't a problem. Please leave a comment to that effect.