[OpenCL patch] kernel attributes implementation

Hi all,

Attached my implementation for work_group_size_hint and vec_type_hint to complete the support for OpenCL kernel attributes (OpenCL spec 1.1, 6.7.2).

The vec_type_hint attribute is somewhat different from the other attributes, since it has a type parameter. To support this, I’ve created a dummy expression type, which is basically a container for a type. I’ll be happy to hear your opinion about this solution, and see if you have alternative solutions.

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

image001.png

kernel-attr.patch (15.8 KB)

Thanks! :slight_smile:

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

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

ping

Any other opinions – reviews?

Thanks

image001.png

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,
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

kernel-attr2.patch (17.1 KB)

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:

I'll take a look today. Many people have been on vacation the last week, so sorry for the delay.

-Tanya

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

kernel-attr3.patch (17.7 KB)