Page MenuHomePhabricator

[OpenCL] Add intel_reqd_sub_group_size attribute support
ClosedPublic

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

Diff Detail

Repository
rL LLVM

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
694–697 ↗(On Diff #91255)

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
2762–2763 ↗(On Diff #91255)

Please, fix indentation.

2774 ↗(On Diff #91255)

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 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
655 ↗(On Diff #91812)

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
655 ↗(On Diff #91812)

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.

Refine some comments.

Anastasia added inline comments.Mar 16 2017, 4:25 AM
lib/CodeGen/CodeGenFunction.cpp
698 ↗(On Diff #91975)

Variable name should start in upper case.

pxli168 added inline comments.Mar 19 2017, 7:27 PM
lib/CodeGen/CodeGenFunction.cpp
698 ↗(On Diff #91975)

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

Fix some varibale up case problems.

Anastasia added inline comments.Apr 6 2017, 11:07 AM
lib/Sema/SemaDeclAttr.cpp
2797 ↗(On Diff #93106)

Could you add this warning to testing as well please.

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

Add attribute test.

@Anastasia is this test added OK?

Fix attribute test case.

This revision was automatically updated to reflect the committed changes.