[OpenCL] Allow address spaces as method qualifiers

[Anastasia]

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.

[Anastasia]

Removed FIXME that has been addressed

[Anastasia]

lib/Sema/SemaOverload.cpp

2828

I am still missing something here.

[rjmccall]

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

6162

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

1157

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.

6671

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

9279

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

[Anastasia]

Address review comments.

lib/Parse/ParseDecl.cpp

6162

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)*)

[Anastasia]

In D55850#1335826, @rjmccall wrote:

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.

[rjmccall]

lib/Parse/ParseDecl.cpp

6162

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

2828

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.

9279

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]

lib/Parse/ParseDecl.cpp

6162

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]

lib/Parse/ParseDecl.cpp

6162

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]

lib/Sema/SemaOverload.cpp

9279

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]

lib/Sema/SemaOverload.cpp

9279

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]

lib/Sema/SemaOverload.cpp

2828

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

[brunodf]

lib/Sema/SemaType.cpp

4877

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.

[Anastasia]

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

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

[Anastasia]

lib/Sema/SemaOverload.cpp

9279

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

lib/Sema/SemaType.cpp

4877

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

[rjmccall]

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

[Anastasia]

In D55850#1358812, @rjmccall wrote:

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

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

[Anastasia]

Fixed wording on the comment.

[rjmccall]

lib/Parse/ParseDecl.cpp

6195

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

3935

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]

• Removed else case in DiagnoseMultipleAddrSpaceAttributes
• Added extra test to check correctness of addr space for 'this' when statically used in a class scope.

[Anastasia]

lib/Parse/ParseDecl.cpp

6195

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]

• Fixed typo in FIXME

[rjmccall]

lib/Parse/ParseDecl.cpp

6195

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

[Anastasia]

Added a comment explaining when multiple address spaces are diagnosed.

[rjmccall]

Okay, LGTM.

[Anastasia]

After rebase I had to modify the following test:

Index: test/SemaOpenCLCXX/address_space_overloading.cl
===================================================================
--- test/SemaOpenCLCXX/address_space_overloading.cl	(revision 351746)
+++ test/SemaOpenCLCXX/address_space_overloading.cl	(working copy)
@@ -1,12 +1,12 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=c++
 
-// expected-no-diagnostics
+// FIXME: This test shouldn't trigger any errors.
 
 struct RetGlob {
   int dummy;
 };
 
-struct RetGen {
+struct RetGen { //expected-error{{binding value of type '__generic RetGen' to reference to type 'RetGen' drops <<ERROR>> qualifiers}}
   char dummy;
 };
 
@@ -19,5 +19,5 @@
   __local int *ArgLoc;
   RetGlob TestGlob = foo(ArgGlob);
   RetGen TestGen = foo(ArgGen);
-  TestGen = foo(ArgLoc);
+  TestGen = foo(ArgLoc); //expected-note{{in implicit copy assignment operator for 'RetGen' first required here}}
 }

After looking at it, I realized that there is another un-handled path for address spaces in the initialization sequence. That gets hit during the creation of a return statement when we define implicit copy assignment. assignment. As a result I end up with the following incorrect AST:

UnaryOperator 0x75a910 '__generic struct RetGen' lvalue prefix '*' cannot overflow
`-CXXThisExpr 0x75a900 '__generic struct RetGen *' this

As I wasn't sure whether I should fix the initialization sequence again by splitting the address space conversion to move it to a later step or just fix the type of *this expr, I will upload a separate fix for this instead of reopening this review.

The overloading resolution seems to work fine at least which is what this test is supposed to check.

[ebevhan]

I'm a bit late to the party here, it was an older patch so I wasn't watching this one.

I get a bit miffed when address space related features get locked behind langoptions that aren't strictly address spaces. I know that there are currently a couple code snippets which are behind LangOptions.OpenCL, that are needed for address spaces to work reasonably even when you aren't using OpenCL.

I guess I do understand that the only address spaces that are interesting to parse here are the named qualifier ones, but it would be convenient if the parsing would accept other ones as well (such as the __attribute__ based ones) and not necessarily assume that the user is using OpenCL++.

EDIT: Sort of jumped the gun there... The summary even says "This patch doesn't enable the feature for C++ yet but it can easily be generalized if the overall flow is right." Don't mind me.

[Anastasia]

In D55850#1365437, @ebevhan wrote:

I'm a bit late to the party here, it was an older patch so I wasn't watching this one.

I get a bit miffed when address space related features get locked behind langoptions that aren't strictly address spaces. I know that there are currently a couple code snippets which are behind LangOptions.OpenCL, that are needed for address spaces to work reasonably even when you aren't using OpenCL.

I guess I do understand that the only address spaces that are interesting to parse here are the named qualifier ones, but it would be convenient if the parsing would accept other ones as well (such as the __attribute__ based ones) and not necessarily assume that the user is using OpenCL++.

EDIT: Sort of jumped the gun there... The summary even says "This patch doesn't enable the feature for C++ yet but it can easily be generalized if the overall flow is right." Don't mind me.

This is correct. My plan was to generalize to C++ in the next patch. As I am more familiar with OpenCL and many rules are already well defined, this was my starting point. However, I want this work to be used for C++ based implementations generically and not just for OpenCL. I am still struggling to understand the semantic of address spaces in C++ for some cases and need a little bit more time, but I think I should be able to upload the initial patch soon. I would then appreciate any help with reviewing to get it right!

[ebevhan]

Okay, sounds good! I'm not a C++ expert, but I'd be happy to look at the patches when they're up. Haven't done much serious testing on my end so far, but from what I've seen the patches so far look good!

[Anastasia]

In D55850#1366094, @ebevhan wrote:

Okay, sounds good! I'm not a C++ expert, but I'd be happy to look at the patches when they're up. Haven't done much serious testing on my end so far, but from what I've seen the patches so far look good!

Cool! Thanks a lot for your feedback btw. Let me know if you discover any issues/bugs! I do need to improve testing at some point soon as I am sure there are still a lot of uncaught corner cases.

[rjmccall]

I'll just add that we've been trying to not put things behind OpenCL guards as much as possible. Most of the remaining OpenCL checks are for OpenCL-specific logic like inferring the default address space, and yeah, we could probably make that a target option or something.

[Anastasia]

In D55850#1366709, @rjmccall wrote:

Most of the remaining OpenCL checks are for OpenCL-specific logic like inferring the default address space, and yeah, we could probably make that a target option or something.

We have an address space map in the targets that can be used to map default address space if needed. Or are you suggesting to migrate functionality like deduceOpenCLImplicitAddrSpace() to the target setting? Although I feel this belong to the language rather.

[rjmccall]

In D55850#1368767, @Anastasia wrote:

In D55850#1366709, @rjmccall wrote:

Most of the remaining OpenCL checks are for OpenCL-specific logic like inferring the default address space, and yeah, we could probably make that a target option or something.

We have an address space map in the targets that can be used to map default address space if needed. Or are you suggesting to migrate functionality like deduceOpenCLImplicitAddrSpace() to the target setting? Although I feel this belong to the language rather.

True. But we could make it a Sema-wide setting, configured immediately so that we don't have to repeatedly check a language option whenever we parse or synthesize a method. It would make that logic feel less language-specific.

[ebevhan]

I think the code that comes to mind is mostly like in GetTypeSourceInfoForDeclarator:

LangAS AS =
    (ASIdx == LangAS::Default ? LangAS::opencl_generic : ASIdx);

It's behind an OpenCLCPlusPlus guard so it won't add generic on anything if it's not OpenCL++, but there shouldn't be a reason why the rest of the code in that block won't work for regular C++.

In fact, in most regular C++ this would be an issue, since Default typically _is_ the 'generic' address space there.

[ebevhan]

lib/Sema/SemaOverload.cpp

5124

I tested this patch with some kludges to let it parse our address space qualifiers, and hit a problem here; just because the FromType isn't qualified doesn't mean that the object conversion is okay. A conversion of an object Type to __X Type might not be legal if there is no conversion from 'no address space' to '__X' address space.

The example was a class with an AS-qualified overload, and when resolving for calling the overloaded method on an object Type *, it hit an ambiguous resolution since it considered the AS-qualified method to be a legal candidate.

It feels like this might be rather specific to how a language/target wants to use address spaces, though.

[Anastasia]

In D55850#1369008, @ebevhan wrote:

I think the code that comes to mind is mostly like in GetTypeSourceInfoForDeclarator:

LangAS AS =
    (ASIdx == LangAS::Default ? LangAS::opencl_generic : ASIdx);

It's behind an OpenCLCPlusPlus guard so it won't add generic on anything if it's not OpenCL++, but there shouldn't be a reason why the rest of the code in that block won't work for regular C++.

In fact, in most regular C++ this would be an issue, since Default typically _is_ the 'generic' address space there.

Yes, opencl_generic is something quite specific to OpenCL I feel. I am not sure we can generalize it to C++. I would quite like to understand this better. However, I think we could potentially implement opencl_generic using Default if only earlier OpenCL implementations wouldn't use Default for opencl_private. And because everything was built on top of this assumption it's now a big and a scary refactoring change to make. However, I think we can align different language modes better if opencl_generic could be implemented as Default. I might look into this at some point.