[PATCH] D16686: [OpenCL] Generate metadata for opencl_unroll_hint attribute

[Anastasia Stulova via cfe-commits]

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