Anastasia added inline comments.
================
Comment at: include/clang/Basic/Attr.td:661
@@ +660,3 @@
+ let Args = [UnsignedArgument<"UnrollHint">];
+ let Documentation = [Undocumented];
+}
----------------
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!
================
Comment at: lib/CodeGen/CGLoopInfo.cpp:126
@@ -122,2 +125,3 @@
- auto *ValueExpr = LH->getValue();
+ LoopHintAttr::OptionType Option = LoopHintAttr::Unroll;
+ LoopHintAttr::LoopHintState State = LoopHintAttr::Disable;
----------------
I would like to see a bit of comments here and may be reference to spec if
possible!
================
Comment at: lib/Parse/ParseStmt.cpp:111
@@ -110,1 +110,3 @@
+ if (getLangOpts().OpenCL && getLangOpts().OpenCLVersion >= 120) {
+ bool wasAttribute = Tok.is(tok::kw___attribute);
----------------
May be it's better to wrap in into MaybeParseOpenCLAttributes function to be
consistent with the rest?
================
Comment at: lib/Parse/ParseStmt.cpp:117
@@ +116,3 @@
+ if (!(Tok.is(tok::kw_for) || Tok.is(tok::kw_while) ||
Tok.is(tok::kw_do))) {
+ AttributeList *attrList = Attrs.getList();
+ assert(attrList != NULL);
----------------
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...
================
Comment at: lib/Parse/ParseStmt.cpp:118
@@ +117,3 @@
+ AttributeList *attrList = Attrs.getList();
+ assert(attrList != NULL);
+ if (attrList->getName()->getName() == "opencl_unroll_hint") {
----------------
Assertion should have a message!
NULL -> nullptr
================
Comment at: lib/Sema/SemaStmtAttr.cpp:210
@@ +209,3 @@
+
+ // opencl_unroll_hint can have 0 arguments (compiler determines unrolling
+ // factor) or 1 argument (the unroll factor provided by the user).
----------------
Could you put a reference to spec here?
================
Comment at: lib/Sema/SemaStmtAttr.cpp:213
@@ +212,3 @@
+
+ unsigned numArgs = A.getNumArgs();
+
----------------
Variable names are not inline with the coding style.
Please, see:
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
================
Comment at: lib/Sema/SemaStmtAttr.cpp:224
@@ +223,3 @@
+ Expr *E = A.getArgAsExpr(0);
+ assert(E != NULL);
+ llvm::APSInt ArgVal(32);
----------------
Assert should have a message!
================
Comment at: lib/Sema/SemaStmtAttr.cpp:227
@@ +226,3 @@
+
+ if (E->isTypeDependent() || E->isValueDependent() ||
+ !E->isIntegerConstantExpr(ArgVal, S.Context)) {
----------------
Do we need these two checks? Seems C++ related.
================
Comment at: lib/Sema/SemaStmtAttr.cpp:230
@@ +229,3 @@
+ S.Diag(A.getLoc(), diag::err_attribute_argument_type)
+ << A.getName()->getName() << AANT_ArgumentIntegerConstant <<
E->getSourceRange();
+ return 0;
----------------
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
================
Comment at: lib/Sema/SemaStmtAttr.cpp:234
@@ +233,3 @@
+
+ int64_t val = ArgVal.getSExtValue();
+
----------------
could it be just an int type?
================
Comment at: test/Parser/opencl-unroll-hint.cl:20
@@ +19,3 @@
+ int i;
+
+ __attribute__((opencl_unroll_hint))
----------------
could we remove an empty line here please?
================
Comment at: test/Parser/opencl-unroll-hint.cl:23
@@ +22,3 @@
+ do {
+ i = counter();
+ x[i] = i;
----------------
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. :)
http://reviews.llvm.org/D16686
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
