[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:
> > 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.
> 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?
> 
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`.


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

https://reviews.llvm.org/D55850