[cfe-dev] [OpenCL patch] kernel attributes implementation

[Douglas Gregor]

On Jan 6, 2012, at 8:06 AM, Anton Lokhmotov wrote:

> Hi Tanya and Guy,
> 
>> - Add some comments to the function EmitOpenCLKernelMetadata. Maybe one
>> sentence to say what the metadata is and reference to spec.
> Done.
> 
>> - Remove the type specifier diagnostic. I don't think its needed (can
>> you just use the existing one?) and its unrelated to the metadata.
> 
> While I appreciate Guy's concern that:
> 
> __kernel __attribute__((vec_type_hint(mmm))) void foo( void ){}
> 
> should generate a better error message, with his code I still see three
> other confusing messages:
> 
> kernel-attributes-invalid.cl:7:39: error: type name requires a specifier or
> qualifier
> __kernel __attribute__((vec_type_hint(mm))) void foo( void ){}
>                                      ^
> kernel-attributes-invalid.cl:7:39: error: OpenCL requires a type specifier
> for all declarations
> __kernel __attribute__((vec_type_hint(mm))) void foo( void ){}
>                                      ^~~
> kernel-attributes-invalid.cl:7:39: error: expected ')'
> __kernel __attribute__((vec_type_hint(mm))) void foo( void ){}
>                                      ^
>                                      )
> kernel-attributes-invalid.cl:7:44: error: expected identifier or '('
> __kernel __attribute__((vec_type_hint(mm))) void foo( void ){}
> 
> I think we should try to come up with a better solution...
> 
>> - The DummyType is a creative solution. I think we'll need Doug to sign
>> off on that change though.
> Please see below an excerpt from an alternative solution handling the
> vec_type_hint parameter as an identifier (yuck!).  Hopefully, it's enough to
> convince Doug ;).

I'm not particularly thrilled with either solution, because both involve special-purpose hacks.

Let's first characterize how best to approach this problem. First of all, the syntax of the vec_type_hint attribute implies that what we really want to do is parse the attribute argument as a type name (via Parse::ParseTypeName). That's exactly what the attached patch does, which is great. I was hoping that the patch would also eliminate the BuiltinType stuff from Parser::ParseGNUAttributeArgs.

The next step is to get that parsed type information from the parser to semantic analysis. The best way to do so would be for AttributeList to properly support type arguments, because then we could directly pass the type through. A refactor of AttributeList et al to support type arguments would benefit vec_type_hint as well as several other arguments that accept types (e.g., iboutletcollection), and would be the best way to approach this problem. It would maintain type-source information properly, and wouldn't require the allocation of an expression that will effectively be leaked.

DummyTypeExpr will encode the type as an expression to get it through the AttributeList machinery without any refactoring. It will work, and it is expedient, but it's not the right long-term solution for Clang. If we have to go in this direction, we shouldn't introduce a new Expr kind for it. Rather, we should just use OpaqueValueExpr, which can fulfill the same role.

	- Doug


> Cheers,
> Anton.
> 
> 
> +static void handleOpenCLVecTypeHint(Sema &S, Decl *D, const AttributeList
> &Attr)
> +{
> +  if (!Attr.getParameterName()) {
> +    S.Diag(Attr.getLoc(), diag::err_attribute_wrong_number_arguments) << 1;
> +    return;
> +  }
> +
> +  IdentifierInfo *II = Attr.getParameterName();
> +
> +  llvm::StringRef IIStr = II->getName();
> +
> +  ParsedType TypeRep =
> +    S.getTypeName(*II, Attr.getLoc(), 
> +                  S.getScopeForContext(D->getDeclContext()->getParent()));
> +
> +  QualType QT;
> +
> +  if (!TypeRep) {
> +    // Try to recognise keywords
> +    QT = llvm::StringSwitch<QualType>(IIStr)
> +      .Case("char", S.Context.CharTy)
> +      .Case("int", S.Context.IntTy)
> +      .Case("short", S.Context.ShortTy)
> +      .Case("long", S.Context.LongTy)
> +      .Case("float", S.Context.FloatTy)
> +      .Case("double", S.Context.DoubleTy)
> +      .Case("half", S.Context.HalfTy)
> +      .Default(QualType())
> +      ;
> +    if (QT.isNull()) {
> +      S.Diag(Attr.getLoc(), diag::err_attribute_argument_invalid_type) <<
> II;
> +      return;
> +    }
> +  } else QT = TypeRep.get();
> +
> <kernel_optional_attribute_qualifiers.patch>