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

[Benyei, Guy]

Hi all,
Attached the updated patch:
* Fixed Pekka's and Anton's remarks
* Changed kernel metadata format according to Anton's proposal
* Use regular expressions in the test to match metadata nodes
* Removed unrelated constant address space handling

Please review

Thanks
    Guy



-----Original Message-----
From: Pekka Jääskeläinen [mailto:pekka.jaaskelainen at tut.fi] 
Sent: Tuesday, January 03, 2012 17:51
To: Benyei, Guy
Cc: cfe-dev at cs.uiuc.edu
Subject: Re: [cfe-dev] [OpenCL patch] kernel attributes implementation

Hi,

I spotted that the "noinline" attribute for kernels is still
added in TCE's targetinfo. It might not be needed anymore
but I'll check and submit a separate patch if that's the case.
(It prevents inlining also when the kernel function is
called from another kernel function in which case inlining
might be desirable?)

A slight stylistic issue:

   else if(AttrName->isStr("vec_type_hint")) {
+    if(T.get() && !T.isInvalid()) {
+      ExprResult ArgExpr = Actions.ActOnDummyTypeExpr(T.get());
+      ArgExprs.push_back(ArgExpr.release());
+    }
+  }

I think the correct style is to have a whitespace after 'if'?

Similar here:
+      }else {

Otherwise, this looks good to me now (to my eyes that are
not very familiar with the Clang code base).

+1 for committing after these cleanups.

02.01.2012 17:52, Benyei, Guy kirjoitti:
> Hi Pekka,
> Attached the updated patch - this one implements the whole kernel metadata in CodeGenFunction.cpp (also moved the reqd_work_group_size attribute from the tce target info).
>
> Please review this new patch.
>
> Thanks
>     Guy
>
>
> -----Original Message-----
> From: Pekka Jääskeläinen [mailto:pekka.jaaskelainen at tut.fi]
> Sent: Monday, January 02, 2012 13:13
> To: Benyei, Guy
> Cc: cfe-dev at cs.uiuc.edu
> Subject: Re: [cfe-dev] [OpenCL patch] kernel attributes implementation
>
> Hi Guy,
>
> Could you change your patch to implement the attributes for
> all targets like discussed below? Otherwise I have to soon
> resubmit this work to make these features work for all
> targets supported by pocl (which covers all "non-SIMT" LLVM targets
> at the moment) which feels a bit like wasted rework.
>
> I originally implemented the reqd_work_group_size attribute
> as metadata for all targets (instead of the TCE target specific
> implementation) which was rejected. But as the new discussion
> didn't bring any reasons against having these attributes as
> metadata by default, I'd propose resubmitting your patches with
> generic implementation next to the opencl.kernels metadata.
>
> The actual OpenCL implementations can ignore the spurious
> metadata at will or convert it to intrinsics or whatever
> is the best for the case. Certainly the harm should be
> insignificant in such cases. The immediate benefit is that
> these attributes can then be supported by (at least) the pocl
> OpenCL implementation for all LLVM targets.
>
> 20.12.2011 13:12, Benyei, Guy kirjoitti:
>> Hi Pekka,
>>
>> I agree that the right place to implement OpenCL specific metadata
>> shouldn't be in target specific, since the attributes are not target
>> specific. IMO, the right solution would be to implement this metadata
>> in CodeGenFunction.cpp, next to the implementation of the
>> "opencl.kernels" metadata.
>>
>> Anyway, I chosen to implement is for the TCE target only due to the
>> fact that reqd_work_group_size is implemented in this target only. It
>> can be changed, of course.
>>
>> Thanks Guy
>>
>> -----Original Message----- From: cfe-dev-bounces at cs.uiuc.edu
>> [mailto:cfe-dev-bounces at cs.uiuc.edu] On Behalf Of Pekka J??skel?inen
>> Sent: Tuesday, December 20, 2011 12:50 To: cfe-dev at cs.uiuc.edu
>> Subject: Re: [cfe-dev] [OpenCL patch] kernel attributes
>> implementation
>>
>> On 12/20/2011 11:18 AM, Benyei, Guy wrote:
>>> I've implemented the handling of these new attributes in the TCE
>>> target info, since reqd_work_group_size attribute is already
>>> implemented there. I also use this target for testing.
>>
>> Thanks! :)
>>
>> There was some discussion about implementing these OpenCL C kernel
>> attributes some time ago [1]. The discussion somehow died.
>>
>> The question raised there was why cannot we add these by default
>> when compiling OpenCL? This would enable pocl [2] to support these
>> OpenCL features for all LLVM targets. The OpenCL implementations that
>> implement the attributes in some other way can just ignore the
>> metadata (except the required work group size cannot be just ignored,
>> AFAIK).
>>
>> [1]
>> http://lists.cs.uiuc.edu/pipermail/llvmdev/2011-December/046204.html
>> [2] https://launchpad.net/pocl
>>
>
>


-- 
Pekka Jääskeläinen
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kernel-attr3.patch
Type: application/octet-stream
Size: 18075 bytes
Desc: kernel-attr3.patch
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120104/7f702e18/attachment.obj>