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.
| lib/Sema/SemaOverload.cpp | ||
|---|---|---|
| 2828 | 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 | ||
|---|---|---|
| 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. | |
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)*) | |
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.
| 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. | |
| lib/Parse/ParseDecl.cpp | ||
|---|---|---|
| 6162 | I have uploaded a separate patch for this: 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? | |
| 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.
Not that are legal to apply to this. | |
| 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. | |
| 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. | |
| lib/Sema/SemaOverload.cpp | ||
|---|---|---|
| 2828 | I created a patch for this in https://reviews.llvm.org/D56198. | |
| 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. | |
Rebased on top of recent related changes and addressed remaining review comments.
This now depends on: https://reviews.llvm.org/D56735
| 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 | |
| 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? | |
| lib/Parse/ParseDecl.cpp | ||
|---|---|---|
| 6195 | Okay, if this is already diagnosed, it isn't a problem. Please leave a comment to that effect. | |
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.
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!
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.
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.
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.
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.
| 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. | |
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.