This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Add intel_reqd_sub_group_size attribute support
ClosedPublic

Authored by pxli168 on Mar 9 2017, 7:13 PM.

Event Timeline

pxli168 created this revision.Mar 9 2017, 7:13 PM
pxli168 retitled this revision from intel_reqd_sub_group_size to [OpenCL] Add intel_reqd_sub_group_size attribute support .Mar 9 2017, 7:19 PM
pxli168 edited the summary of this revision. (Show Details)
pxli168 added reviewers: Anastasia, bader, hfinkel.
bader accepted this revision.Mar 10 2017, 3:00 AM

LGTM.
Just a few minor issues.

lib/CodeGen/CodeGenFunction.cpp
699–702

It would be nice if you update the comment for EmitOpenCLKernelMetadata function declaration in CodeGenFunction.
I think we should make it more generic and avoid implementation details like listing exact attribute names.

lib/Sema/SemaDeclAttr.cpp
2877–2878

Please, fix indentation.

2889

Please, write your code to fit within 80 columns.

This revision is now accepted and ready to land.Mar 10 2017, 3:00 AM
pxli168 updated this revision to Diff 91812.Mar 14 2017, 8:19 PM
pxli168 retitled this revision from [OpenCL] Add intel_reqd_sub_group_size attribute support to [OpenCL] Add intel_reqd_sub_group_size attribute support.

Fix some minor issues from bader.

bader added inline comments.Mar 15 2017, 1:58 AM
lib/CodeGen/CodeGenFunction.cpp
650

Xuili, sorry for confusion.
I was talking about the declaration in CodeGenFunction.h file. https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/CodeGenFunction.h#L1397

 
/// Add a kernel metadata node to the named metadata node 'opencl.kernels'.
/// In the kernel metadata node, reference the kernel function and metadata 
/// nodes for its optional attribute qualifiers (OpenCL 1.1 6.7.2):
/// - A node for the vec_type_hint(<type>) qualifier contains string
///   "vec_type_hint", an undefined value of the <type> data type,
///   and a Boolean that is true if the <type> is integer and signed.
/// - A node for the work_group_size_hint(X,Y,Z) qualifier contains string 
///   "work_group_size_hint", and three 32-bit integers X, Y and Z.
/// - A node for the reqd_work_group_size(X,Y,Z) qualifier contains string 
///   "reqd_work_group_size", and three 32-bit integers X, Y and Z.

As you see this comment explicitly lists all the metadata added to 'opencl.kernels', which seems to be unnecessary.
Instead of adding new comment about 'intel_reqd_sub_group_size' metadata, I think it would be better just leave the fist line of the comment and remove the rest.

pxli168 added inline comments.Mar 16 2017, 12:46 AM
lib/CodeGen/CodeGenFunction.cpp
650

OK. I will change it in next patch. And I think we no longer add these metadatas to the metadata node 'opencl.kernels'. We have change these to be function metadatas, this comment is out of date.

pxli168 updated this revision to Diff 91975.Mar 16 2017, 12:50 AM

Refine some comments.

Anastasia added inline comments.Mar 16 2017, 4:25 AM
lib/CodeGen/CodeGenFunction.cpp
701

Variable name should start in upper case.

pxli168 added inline comments.Mar 19 2017, 7:27 PM
lib/CodeGen/CodeGenFunction.cpp
701

I think I should also change the whole function here. I just do some copy paste and don't notice this detail.

pxli168 updated this revision to Diff 93106.Mar 27 2017, 2:39 AM

Fix some varibale up case problems.

Anastasia added inline comments.Apr 6 2017, 11:07 AM
lib/Sema/SemaDeclAttr.cpp
2886

Could you add this warning to testing as well please.

pxli168 updated this revision to Diff 94782.Apr 10 2017, 11:46 PM

Add duplicate warning test case.

Anastasia accepted this revision.Apr 19 2017, 11:09 AM

LGTM! Thanks!

pxli168 accepted this revision.Apr 19 2017, 11:25 PM

I found that there is a newly added patch about attribute that will fail the test, and I am working on it.
https://reviews.llvm.org/D30009
https://reviews.llvm.org/D32176

pxli168 updated this revision to Diff 95896.Apr 19 2017, 11:50 PM

Add attribute test.

@Anastasia is this test added OK?

pxli168 updated this revision to Diff 97772.May 3 2017, 10:36 PM

Fix attribute test case.

This revision was automatically updated to reflect the committed changes.