[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/Parse/ParseDecl.cpp:6162
+        }
+      }
+
----------------
Anastasia wrote:
> rjmccall wrote:
> > 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.
> 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)*)
> 
> 
> 
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 `SourceLocation`s 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.


================
Comment at: lib/Sema/SemaOverload.cpp:2828
 
   // FIXME: OpenCL: Need to consider address spaces
   unsigned FromQuals = FromFunction->getTypeQuals().getCVRUQualifiers();
----------------
Anastasia wrote:
> I am still missing something here.
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.


================
Comment at: lib/Sema/SemaOverload.cpp:9279
+        (CandAS1 != LangAS::opencl_generic && CandAS1 != LangAS::Default))
+      return true;
+  }
----------------
rjmccall wrote:
> This at least needs a comment explaining the rule you're trying to implement.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55850/new/

https://reviews.llvm.org/D55850