Page MenuHomePhabricator

[OpenCL] Generate metadata for opencl_unroll_hint attribute
ClosedPublic

Authored by yaxunl on Jan 28 2016, 9:44 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
pxli168 added inline comments.Jan 28 2016, 6:58 PM
include/clang/Basic/DiagnosticParseKinds.td
906 ↗(On Diff #46288)

you can change it to opencl_unroll_hint attribute

Anastasia added inline comments.Jan 29 2016, 9:36 AM
include/clang/Basic/Attr.td
661 ↗(On Diff #46288)

I think undocumented is not allowed any longer.

I suggest you to check OpenCLGenericAddressSpace here and also OpenCLAddressSpaceGenericDocs in AttrDocs.td for an example how it can be done!

lib/CodeGen/CGLoopInfo.cpp
126 ↗(On Diff #46288)

I would like to see a bit of comments here and may be reference to spec if possible!

lib/Parse/ParseStmt.cpp
111 ↗(On Diff #46288)

May be it's better to wrap in into MaybeParseOpenCLAttributes function to be consistent with the rest?

117 ↗(On Diff #46288)

I am just thinking could we avoid checks on line 115 that are a bit complicated to read.

Could we just try to check if the attrList is empty or not. And if not NULL check the name of the attribute etc...

118 ↗(On Diff #46288)

Assertion should have a message!

NULL -> nullptr

lib/Sema/SemaStmtAttr.cpp
210 ↗(On Diff #46288)

Could you put a reference to spec here?

213 ↗(On Diff #46288)

Variable names are not inline with the coding style.

Please, see:
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

224 ↗(On Diff #46288)

Assert should have a message!

227 ↗(On Diff #46288)

Do we need these two checks? Seems C++ related.

230 ↗(On Diff #46288)

This line is too long. Please make sure to format your code correctly.

Run clang-format on the final patch:

http://clang.llvm.org/docs/ClangFormat.html
234 ↗(On Diff #46288)

could it be just an int type?

test/Parser/opencl-unroll-hint.cl
20 ↗(On Diff #46288)

could we remove an empty line here please?

23 ↗(On Diff #46288)

I think similarly to the test kernel below, we don't really need anything in the loop body... just to make the test simpler to understand. :)

yaxunl updated this revision to Diff 46434.Jan 29 2016, 2:37 PM
yaxunl marked 17 inline comments as done.

Revised as Anastasia and Xiuli suggested.

yaxunl updated this revision to Diff 46526.Feb 1 2016, 6:12 AM

update the tests.

Anastasia added inline comments.Feb 1 2016, 10:27 AM
include/clang/Parse/Parser.h
2200 ↗(On Diff #46526)

May be better:
OpenCL 2.0+ -> OpenCL v2.0 or higher

lib/CodeGen/CGLoopInfo.cpp
131 ↗(On Diff #46526)

Just to match the other comment style:

OpenCL 2.0 spec section 6.11.5 -> OpenCL v2.0 s6.11.5

lib/Sema/SemaStmtAttr.cpp
212 ↗(On Diff #46526)

OpenCL v2.0 s6.11.5

221 ↗(On Diff #46526)

Variable names are still not compliant to coding style. Could you please change.

234 ↗(On Diff #46526)

Could you please change the type to int to match similar code in other places.

235 ↗(On Diff #46526)

Also could you change variable name here too and in other places to follow the coding style.

test/CodeGenOpenCL/unroll-hint.cl
9 ↗(On Diff #46526)

I would make all loops with an empty body here too as it doesn't play any role in testing. :)

yaxunl updated this revision to Diff 46570.Feb 1 2016, 12:41 PM
yaxunl marked 7 inline comments as done.

Revised as Anastasia suggested.

pxli168 added inline comments.Feb 1 2016, 6:45 PM
test/SemaOpenCL/unroll-hint.cl
22 ↗(On Diff #46434)

It seems the negative integer invalid test is missing?
I think it was in your last diff.

yaxunl updated this revision to Diff 46608.Feb 1 2016, 7:08 PM

Add test for negative unroll hint value as suggested by Xiuli.

pxli168 added inline comments.Feb 2 2016, 1:26 AM
lib/Sema/SemaStmtAttr.cpp
208 ↗(On Diff #46608)

This is also not necessary because this function can be called only if it is an OpenCLUnrollHint

210 ↗(On Diff #46608)

You may put the reference at the begin and make a summary after that, I don't think see ... for details is need.
Just like what other OpenCL references.

217 ↗(On Diff #46608)

def err_attribute_too_many_arguments : Error<"%0 attribute takes no more than %1 argument%s1">;

you should give this two arguments, one is name and another is the number you expected.

218 ↗(On Diff #46608)

please use nullptr
should not use 0 in C++11.

225 ↗(On Diff #46608)

Is this necessary as you have checked there is only one arg in this attr?

230 ↗(On Diff #46608)

Why there is two getName?

test/SemaOpenCL/unroll-hint.cl
17 ↗(On Diff #46608)

I think

expected-error {{1 attribute takes no more than 1 argument}}

should be opencl_unroll_hint

Anastasia added inline comments.Feb 2 2016, 4:19 AM
lib/Sema/SemaStmtAttr.cpp
225 ↗(On Diff #46608)

I guess this might be the case if it's not an Expr, but not sure what else it could be. I think we probably don't need it.

230 ↗(On Diff #46608)

1st returns IdentifierInfo and 2nd actual StringRef.

yaxunl updated this revision to Diff 46651.Feb 2 2016, 7:58 AM
yaxunl marked 9 inline comments as done.

Revised as Xiuli and Anastasia suggested.

pxli168 added inline comments.Feb 2 2016, 6:24 PM
lib/Sema/SemaStmtAttr.cpp
216 ↗(On Diff #46651)

I think you should not use hard code here, just use A.getName(). And diag will find a good way to output is.

230 ↗(On Diff #46651)

I think diag did not need StringRef, A.getName() is enough. It is Diag's job to find the best output form.

yaxunl updated this revision to Diff 46783.Feb 3 2016, 7:38 AM
yaxunl marked an inline comment as done.

Revised as Xiuli suggested.

With the change, there will be quotation marks around the attribute name, so other diagnostics are also changed to be consistent.

Anastasia added inline comments.Feb 3 2016, 10:37 AM
lib/Sema/SemaStmtAttr.cpp
230 ↗(On Diff #46783)

0 -> nullptr

yaxunl updated this revision to Diff 46838.Feb 3 2016, 2:22 PM
yaxunl marked 3 inline comments as done.

Revised as Anastasia suggested.

pxli168 added inline comments.Feb 3 2016, 6:52 PM
lib/CodeGen/CGLoopInfo.cpp
131 ↗(On Diff #46838)

Can you also change this comment to the style that reference go first then summary.

lib/Sema/SemaStmtAttr.cpp
224 ↗(On Diff #46838)

Should this assert be here?
Can it be something else?
And you shuold just use assert(E && ....).

test/SemaOpenCL/unroll-hint.cl
1 ↗(On Diff #46838)

What will happen if using OpenCL 1.2 or 1.1 that earlier than 2.0?
You can also make a test for it, and can change this test case to
unroll-hint-cl2.0.cl
as you can see other cl2.0 features in the folder.

yaxunl updated this revision to Diff 46961.Feb 4 2016, 2:11 PM
yaxunl updated this object.
yaxunl marked 3 inline comments as done.

Revised as Xiuli suggested.
Add test for OpenCL below 2.0.
Add diagnostic for opencl_unroll_hint attribute used below OpenCL 2.0.

pxli168 edited edge metadata.Feb 4 2016, 7:38 PM

Hi Pekka/Anastasia,

I find that most of the attribute parses was done in ParseDecl.cpp, should this also be in there?

Thanks
Xiuli

lib/Parse/ParseStmt.cpp
2214 ↗(On Diff #46961)

I am wondering where should this function goes in, here or ParseDecl.cpp.

2220 ↗(On Diff #46961)

Using string is not very strict, can use AttributeList Kind to check that.

Attrs.getList()->getKind() != AttributeList::AT_OpenCLUnrollHint
test/SemaOpenCL/unroll-hint.cl
1 ↗(On Diff #46961)

I think one test case here is enough, since they all hint the same code.

Anastasia added inline comments.Feb 5 2016, 7:50 AM
lib/Parse/ParseStmt.cpp
2214 ↗(On Diff #46961)

ParseGNUAttributes is in ParseDecl.cpp, but may be because it's generally applicable to Decls?

I am not aware of any rule here.

Hi richard,

What is your opinion about where to put this ParseOpenCLUnrollHintAttribute?

Thanks
Xiuli

lib/Parse/ParseStmt.cpp
2214 ↗(On Diff #46961)

Maybe we could ask richard for help?

Anastasia added inline comments.Feb 9 2016, 10:04 AM
lib/Parse/ParseStmt.cpp
2214 ↗(On Diff #46961)

I am thinking of leaving it here for now as we are almost done.

We can always do fixes later on!

2220 ↗(On Diff #46961)

ping!

test/SemaOpenCL/unroll-hint.cl
1 ↗(On Diff #46961)

ping!

yaxunl updated this revision to Diff 47337.Feb 9 2016, 10:27 AM
yaxunl edited edge metadata.
yaxunl marked 4 inline comments as done.

Sorry for the delay.

Revised as Xiuli suggested.

Anastasia added inline comments.Feb 10 2016, 7:29 AM
test/SemaOpenCL/unroll-hint-cl20.cl
1 ↗(On Diff #47337)

Could we just combine this with test/SemaOpenCL/unroll-hint.cl.

You can have two RUN lines and pass something like -DCL20 in one of them (for CL2.0). It will then allow you to #ifdef the CL2.0 code. You can see the example in:

test/Parser/opencl-atomics-cl20.cl
yaxunl updated this revision to Diff 47460.Feb 10 2016, 8:36 AM

merge two sema tests as one as Anastasia suggested.

Anastasia accepted this revision.Feb 11 2016, 10:40 AM
Anastasia edited edge metadata.

LGTM!

lib/CodeGen/CGLoopInfo.cpp
134 ↗(On Diff #47460)

. missing

This revision is now accepted and ready to land.Feb 11 2016, 10:40 AM
yaxunl updated this revision to Diff 47716.Feb 11 2016, 1:14 PM
yaxunl edited edge metadata.
yaxunl marked an inline comment as done.

Revised as Anastasia suggested.

Changed comments.

@pekka, Xiuli, Richard, any more comments here?

Thanks!

pekka.jaaskelainen edited edge metadata.

LGTM.

pxli168 accepted this revision.Feb 17 2016, 4:40 PM
pxli168 edited edge metadata.

Sorry for the late reply, I was on a break.
LGTM!

yaxunl removed a reviewer: rsmith.Feb 18 2016, 10:31 AM
yaxunl added a subscriber: rsmith.
This revision was automatically updated to reflect the committed changes.