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

[Tanya Lattner]

On Jan 4, 2012, at 11:44 AM, Tanya Lattner wrote:

> Guy & Anton,
> 
> I'll just review this merged patch. Here is my feedback:
> 
> - Remove the type specifier diagnostic. I don't think its needed (can you just use the existing one?) and its unrelated to the metadata.
> 
> - I'd like to suggest changing how the metadata is organized. So it would be like this:
> NamedMetaData Node - opencl.kernels
> 	MDNode Pair of Function*, MDNode which is a list of all the metadata
> 		MDNode list that is a key/value pair. So for each attribute you have key (ie. "vec_type_hint"), and then the value(s).
> 
> Its just a slight variation on how you have things (where you have a list with the first arg being the key). This makes it pretty obvious what the layout is in my opinion.
> 
> Does that seem reasonable?

Actually, let me clarify this a bit.

So I think the layout you have is this:

NamedMDNode(Fn*, X number of MDNodes..), where each MDNode node is a ("key" (MDString), X number of value*).

If so, then that is what I'm suggesting as well except I was nesting the lists (at each level) in an MDNode. Not sure if thats really beneficial now that I think about it :)

So, I retract my opinion here.

Thanks,
Tanya

> 
> - The DummyType is a creative solution. I think we'll need Doug to sign off on that change though.
> 
> - Add some comments to the function EmitOpenCLKernelMetadata. Maybe one sentence to say what the metadata is and reference to spec.
> 
> - Nit pick: Add periods to your comments in AttributeList.h, 
> 
> 
> Thanks,
> Tanya
> 		
> 
> On Jan 4, 2012, at 8:46 AM, Anton Lokhmotov wrote:
> 
>> Hi Guy,
>> 
>> We've done the merge (including your most recent test using regular
>> expressions).  Please review if anything is missing.
>> 
>> I don't quite understand this change (in lib/Sema/SemaType.cpp):
>> 
>> +      } else if (S.getLangOptions().OpenCL) {
>> +        S.Diag(DeclLoc, diag::err_missing_type_specifier_opencl)
>> +          << DS.getSourceRange();
>> +        declarator.setInvalidType(true);
>> 
>> Could you please explain how it relates to the kernel attributes?
>> 
>> Cheers,
>> Anton.<kernel_attributes.patch>
>