[OpenCL] Generate metadata for opencl_unroll_hint attribute

[yaxunl]

Summary

Add codegen and diagnostics for opencl_unroll_hint attribute as OpenCL v2.0 s6.11.5.

The code is based on Khronos OpenCL compiler (https://github.com/KhronosGroup/SPIR/tree/spirv-1.0).

Differential Revision: http://reviews.llvm.org/D16686

[pxli168]

include/clang/Basic/DiagnosticSemaKinds.td

7661

opencl_unroll_hint attribute

lib/Sema/SemaStmtAttr.cpp

216

miss a test to hit this.

test/Parser/opencl-unroll-hint.cl

8

Seems only this is a parser check?
You should put others in test/SemaOpenCL/

[pxli168]

include/clang/Basic/DiagnosticParseKinds.td

906

you can change it to opencl_unroll_hint attribute

[Anastasia]

include/clang/Basic/Attr.td

661

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–127

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

lib/Parse/ParseStmt.cpp

111

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

117

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

Assertion should have a message!

NULL -> nullptr

lib/Sema/SemaStmtAttr.cpp

210

Could you put a reference to spec here?

213

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

Assert should have a message!

227

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

230

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

could it be just an int type?

test/Parser/opencl-unroll-hint.cl

21

could we remove an empty line here please?

24

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]

Revised as Anastasia and Xiuli suggested.

[yaxunl]

update the tests.

[Anastasia]

include/clang/Parse/Parser.h

2200

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

lib/CodeGen/CGLoopInfo.cpp

131

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

OpenCL v2.0 s6.11.5

221

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

234

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

235

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

test/CodeGenOpenCL/unroll-hint.cl

10

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

[yaxunl]

Revised as Anastasia suggested.

[pxli168]

test/SemaOpenCL/unroll-hint.cl

22

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

[yaxunl]

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

[pxli168]

lib/Sema/SemaStmtAttr.cpp

208

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

210

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

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

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

225

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

230

Why there is two getName?

test/SemaOpenCL/unroll-hint.cl

18

I think

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

should be opencl_unroll_hint

[Anastasia]

lib/Sema/SemaStmtAttr.cpp

225

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

1st returns IdentifierInfo and 2nd actual StringRef.

[yaxunl]

Revised as Xiuli and Anastasia suggested.

[pxli168]

lib/Sema/SemaStmtAttr.cpp

216

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

230

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

[yaxunl]

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]

lib/Sema/SemaStmtAttr.cpp

230

0 -> nullptr

[yaxunl]

Revised as Anastasia suggested.

[pxli168]

lib/CodeGen/CGLoopInfo.cpp

131

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

lib/Sema/SemaStmtAttr.cpp

224

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

test/SemaOpenCL/unroll-hint.cl

2

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]

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

[pxli168]

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

2230

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

2236

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

Attrs.getList()->getKind() != AttributeList::AT_OpenCLUnrollHint

test/SemaOpenCL/unroll-hint.cl

2

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

[Anastasia]

lib/Parse/ParseStmt.cpp

2230

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

I am not aware of any rule here.

[pxli168]

Hi richard,

What is your opinion about where to put this ParseOpenCLUnrollHintAttribute?

Thanks
Xiuli

lib/Parse/ParseStmt.cpp

2230

Maybe we could ask richard for help?

[Anastasia]

lib/Parse/ParseStmt.cpp

2230

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

We can always do fixes later on!

2236

ping!

test/SemaOpenCL/unroll-hint.cl

2

ping!

[yaxunl]

Sorry for the delay.

Revised as Xiuli suggested.

[Anastasia]

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]

merge two sema tests as one as Anastasia suggested.

[Anastasia]

LGTM!

lib/CodeGen/CGLoopInfo.cpp

134

. missing

[yaxunl]

Revised as Anastasia suggested.

Changed comments.

[Anastasia]

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

Thanks!

[pekka.jaaskelainen]

LGTM.

[yaxunl]

ping

[pxli168]

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

include/clang/Basic/Attr.td

   let Documentation = [Undocumented];
 }
 
+def OpenCLUnrollHint : InheritableAttr {
+  let Spellings = [GNU<"opencl_unroll_hint">];
+  let Args = [UnsignedArgument<"UnrollHint">];
+  let Documentation = [OpenCLUnrollHintDocs];
+}
+
 // This attribute is both a type attribute, and a declaration attribute (for
 // parameter variables).
 def OpenCLImageAccess : Attr {

include/clang/Basic/AttrDocs.td

   }];
 }
 
+def OpenCLUnrollHintDocs : Documentation {
+  let Category = DocCatStmt;
+  let Heading = "__attribute__((opencl_unroll_hint))";
+  let Content = [{
+The opencl_unroll_hint attribute qualifier can be used to specify that a loop
+(for, while and do loops) can be unrolled. This attribute qualifier can be
+used to specify full unrolling or partial unrolling by a specified amount.
+This is a compiler hint and the compiler may ignore this directive. See
+`OpenCL 2.0 spec <https://www.khronos.org/registry/cl/specs/opencl-2.0.pdf>`_
+section 6.11.5 for details.
+  }];
+}
+
 def DocOpenCLAddressSpaces : DocumentationCategory<"OpenCL Address Spaces"> {
   let Content = [{
 The address space qualifier may be used to specify the region of memory that is

include/clang/Basic/DiagnosticParseKinds.td

 def err_pragma_optimize_extra_argument : Error<
   "unexpected extra argument '%0' to '#pragma clang optimize'">;
 
+def err_opencl_unroll_hint_on_non_loop : Error<
+  "OpenCL only supports opencl_unroll_hint attribute on for, while, and do statements">;
+
 // OpenCL EXTENSION pragma (OpenCL 1.1 [9.1])
 def warn_pragma_expected_colon : Warning<
   "missing ':' after %0 - ignoring">, InGroup<IgnoredPragmas>;

include/clang/Basic/DiagnosticSemaKinds.td

   "the event_t type can only be used with __private address space qualifier">;
 def err_expected_kernel_void_return_type : Error<
   "kernel must have void return type">;
+def err_opencl_unroll_hint_factor : Error<
+  "opencl_unroll_hint attribute requires a positive integral compile time constant expression">;
 def err_sampler_argument_required : Error<
   "sampler_t variable required - got %0">;
 def err_wrong_sampler_addressspace: Error<

include/clang/Parse/Parser.h

   SourceLocation SkipExtendedMicrosoftTypeAttributes();
   void ParseMicrosoftInheritanceClassAttributes(ParsedAttributes &attrs);
   void ParseBorlandTypeAttributes(ParsedAttributes &attrs);
-  void ParseOpenCLAttributes(ParsedAttributes &attrs);
+  void ParseOpenCLKernelAttributes(ParsedAttributes &attrs);
   void ParseOpenCLQualifiers(ParsedAttributes &Attrs);
+  /// \brief Parses opencl_unroll_hint attribute if language is OpenCL 2.0+.
+  /// \return false if error happens.
+  bool MaybeParseOpenCLUnrollHintAttribute(ParsedAttributes &Attrs) {
+    if (getLangOpts().OpenCL && getLangOpts().OpenCLVersion >= 200)
+      return ParseOpenCLUnrollHintAttribute(Attrs);
+    return true;
+  }
+  /// \brief Parses opencl_unroll_hint attribute.
+  /// \return false if error happens.
+  bool ParseOpenCLUnrollHintAttribute(ParsedAttributes &Attrs);
   void ParseNullabilityTypeSpecifiers(ParsedAttributes &attrs);
 
   VersionTuple ParseVersionTuple(SourceRange &Range);

lib/CodeGen/CGLoopInfo.cpp

   // Identify loop hint attributes from Attrs.
   for (const auto *Attr : Attrs) {
     const LoopHintAttr *LH = dyn_cast<LoopHintAttr>(Attr);
+    const OpenCLUnrollHintAttr *OpenCLHint =
+        dyn_cast<OpenCLUnrollHintAttr>(Attr);
 
     // Skip non loop hint attributes
-    if (!LH)
+    if (!LH && !OpenCLHint) {
       continue;
+    }
 
-    auto *ValueExpr = LH->getValue();
+    LoopHintAttr::OptionType Option = LoopHintAttr::Unroll;
+    LoopHintAttr::LoopHintState State = LoopHintAttr::Disable;
     unsigned ValueInt = 1;
-    if (ValueExpr) {
-      llvm::APSInt ValueAPS = ValueExpr->EvaluateKnownConstInt(Ctx);
-      ValueInt = ValueAPS.getSExtValue();
-    }
+    // Translate opencl_unroll_hint attribute argument to
+    // equivalent LoopHintAttr enums.
+    // See OpenCL 2.0 spec section 6.11.5 for details.
+    // 0 corresponds to opencl_unroll_hint attribute without
+    // argument.
+    // 0 - full unroll
+    // 1 - disable unroll
+    // other positive integer n - unroll by n
+    if (OpenCLHint) {
+      ValueInt = OpenCLHint->getUnrollHint();
+      if (ValueInt == 0) {
+        State = LoopHintAttr::Full;
+      } else if (ValueInt != 1) {
+        Option = LoopHintAttr::UnrollCount;
+        State = LoopHintAttr::Numeric;
+      }
+    } else if (LH) {
+      auto *ValueExpr = LH->getValue();
+      if (ValueExpr) {
+        llvm::APSInt ValueAPS = ValueExpr->EvaluateKnownConstInt(Ctx);
+        ValueInt = ValueAPS.getSExtValue();
+      }
 
-    LoopHintAttr::OptionType Option = LH->getOption();
-    LoopHintAttr::LoopHintState State = LH->getState();
+      Option = LH->getOption();
+      State = LH->getState();
+    }
     switch (State) {
     case LoopHintAttr::Disable:
       switch (Option) {

lib/Parse/ParseDecl.cpp

   }
 }
 
-void Parser::ParseOpenCLAttributes(ParsedAttributes &attrs) {
+void Parser::ParseOpenCLKernelAttributes(ParsedAttributes &attrs) {
   // Treat these like attributes
   while (Tok.is(tok::kw___kernel)) {
     IdentifierInfo *AttrName = Tok.getIdentifierInfo();

 
     // OpenCL single token adornments.
     case tok::kw___kernel:
-      ParseOpenCLAttributes(DS.getAttributes());
+      ParseOpenCLKernelAttributes(DS.getAttributes());
       continue;
 
     // Nullability type specifiers.

lib/Parse/ParseStmt.cpp

 
   ParsedAttributesWithRange Attrs(AttrFactory);
   MaybeParseCXX11Attributes(Attrs, nullptr, /*MightBeObjCMessageSend*/ true);
+  if (!MaybeParseOpenCLUnrollHintAttribute(Attrs))
+    return StmtError();
 
   StmtResult Res = ParseStatementOrDeclarationAfterAttributes(
       Stmts, Allowed, TrailingElseLoc, Attrs);

   }
   Braces.consumeClose();
 }
+
+bool Parser::ParseOpenCLUnrollHintAttribute(ParsedAttributes &Attrs) {
+  MaybeParseGNUAttributes(Attrs);
+
+  if (Attrs.empty())
+    return true;
+
+  if (Attrs.getList()->getName()->getName() != "opencl_unroll_hint")
+    return true;
+
+  if (!(Tok.is(tok::kw_for) || Tok.is(tok::kw_while) || Tok.is(tok::kw_do))) {
+    Diag(Tok, diag::err_opencl_unroll_hint_on_non_loop);
+    return false;
+  }
+  return true;
+}

lib/Sema/SemaStmtAttr.cpp

   }
 }
 
+static Attr *handleOpenCLUnrollHint(Sema &S, Stmt *St, const AttributeList &A,
+                                    SourceRange Range) {
+  assert(A.getKind() == AttributeList::AT_OpenCLUnrollHint);
+
+  // opencl_unroll_hint can have 0 arguments (compiler determines unrolling
+  // factor) or 1 argument (the unroll factor provided by the user).
+  // See OpenCL 2.0 spec section 6.11.5 for details.
+
+  unsigned NumArgs = A.getNumArgs();
+
+  if (NumArgs > 1) {
+    S.Diag(A.getLoc(), diag::err_attribute_too_many_arguments) << 1;
+    return 0;
+  }
+
+  unsigned unrollingFactor = 0;
+
+  if (NumArgs == 1) {
+    Expr *E = A.getArgAsExpr(0);
+    assert(E != nullptr && "Invalid opencl_unroll_hint argument");
+    llvm::APSInt ArgVal(32);
+
+    if (!E->isIntegerConstantExpr(ArgVal, S.Context)) {
+      S.Diag(A.getLoc(), diag::err_attribute_argument_type)
+          << A.getName()->getName() << AANT_ArgumentIntegerConstant
+          << E->getSourceRange();
+      return 0;
+    }
+
+    int64_t val = ArgVal.getSExtValue();
+
+    if (val <= 0) {
+      S.Diag(A.getRange().getBegin(), diag::err_opencl_unroll_hint_factor);
+      return 0;
+    }
+    unrollingFactor = val;
+  }
+
+  return OpenCLUnrollHintAttr::CreateImplicit(S.Context, unrollingFactor);
+}
+
 static Attr *ProcessStmtAttribute(Sema &S, Stmt *St, const AttributeList &A,
                                   SourceRange Range) {
   switch (A.getKind()) {

     return handleFallThroughAttr(S, St, A, Range);
   case AttributeList::AT_LoopHint:
     return handleLoopHintAttr(S, St, A, Range);
+  case AttributeList::AT_OpenCLUnrollHint:
+    return handleOpenCLUnrollHint(S, St, A, Range);
   default:
     // if we're here, then we parsed a known attribute, but didn't recognize
     // it as a statement attribute => it is declaration attribute

test/CodeGenOpenCL/unroll-hint.cl

+// RUN: %clang_cc1 -emit-llvm -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -O0 -cl-std=CL1.2 -o - %s | FileCheck %s
+
+/*** for ***/
+void for_count(int* sum)
+{
+// CHECK-LABEL: for_count
+    __attribute__((opencl_unroll_hint(8)))
+    for( int i = 0; i < 1000; ++i) {
+        *sum += i;
+    }
+// CHECK: br label %{{.*}}, !llvm.loop ![[FOR_COUNT:.*]]
+}
+
+void for_disable(int* sum)
+{
+// CHECK-LABEL: for_disable
+    __attribute__((opencl_unroll_hint(1)))
+    for( int i = 0; i < 1000; ++i) {
+        *sum += i;
+    }
+// CHECK: br label %{{.*}}, !llvm.loop ![[FOR_DISABLE:.*]]
+}
+
+void for_full(int* sum)
+{
+// CHECK-LABEL: for_full
+    __attribute__((opencl_unroll_hint))
+    for( int i = 0; i < 1000; ++i) {
+        *sum += i;
+    }
+// CHECK: br label %{{.*}}, !llvm.loop ![[FOR_FULL:.*]]
+}
+
+/*** while ***/
+void while_count(int* sum)
+{
+// CHECK-LABEL: while_count
+    int i = 1000;
+    __attribute__((opencl_unroll_hint(8)))
+    while(i-->0) {
+        *sum += i;
+    }
+// CHECK: br label %{{.*}}, !llvm.loop ![[WHILE_COUNT:.*]]
+}
+
+void while_disable(int* sum)
+{
+// CHECK-LABEL: while_disable
+    int i = 1000;
+    __attribute__((opencl_unroll_hint(1)))
+    while(i-->0) {
+        *sum += i;
+    }
+// CHECK: br label %{{.*}}, !llvm.loop ![[WHILE_DISABLE:.*]]
+}
+
+void while_full(int* sum)
+{
+// CHECK-LABEL: while_full
+    int i = 1000;
+    __attribute__((opencl_unroll_hint))
+    while(i-->0) {
+        *sum += i;
+    }
+// CHECK: br label %{{.*}}, !llvm.loop ![[WHILE_FULL:.*]]
+}
+
+/*** do ***/
+void do_count(int* sum)
+{
+// CHECK-LABEL: do_count
+    int i = 1000;
+    __attribute__((opencl_unroll_hint(8)))
+    do {
+        *sum += i;
+    } while(i--> 0);
+// CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !llvm.loop ![[DO_COUNT:.*]]
+}
+
+void do_disable(int* sum)
+{
+// CHECK-LABEL: do_disable
+    int i = 1000;
+    __attribute__((opencl_unroll_hint(1)))
+    do {
+        *sum += i;
+    } while(i--> 0);
+// CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !llvm.loop ![[DO_DISABLE:.*]]
+}
+
+void do_full(int* sum)
+{
+// CHECK-LABEL: do_full
+    int i = 1000;
+    __attribute__((opencl_unroll_hint))
+    do {
+        *sum += i;
+    } while(i--> 0);
+// CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !llvm.loop ![[DO_FULL:.*]]
+}
+
+
+// CHECK: ![[FOR_COUNT]]     =  distinct !{![[FOR_COUNT]],  ![[COUNT:.*]]}
+// CHECK: ![[COUNT]]         =  !{!"llvm.loop.unroll.count", i32 8}
+// CHECK: ![[FOR_DISABLE]]   =  distinct !{![[FOR_DISABLE]],  ![[DISABLE:.*]]}
+// CHECK: ![[DISABLE]]       =  !{!"llvm.loop.unroll.disable"}
+// CHECK: ![[FOR_FULL]]      =  distinct !{![[FOR_FULL]],  ![[FULL:.*]]}
+// CHECK: ![[FULL]]          =  !{!"llvm.loop.unroll.full"}
+// CHECK: ![[WHILE_COUNT]]   =  distinct !{![[WHILE_COUNT]],    ![[COUNT]]}
+// CHECK: ![[WHILE_DISABLE]] =  distinct !{![[WHILE_DISABLE]],  ![[DISABLE]]}
+// CHECK: ![[WHILE_FULL]]    =  distinct !{![[WHILE_FULL]],     ![[FULL]]}
+// CHECK: ![[DO_COUNT]]      =  distinct !{![[DO_COUNT]],       ![[COUNT]]}
+// CHECK: ![[DO_DISABLE]]    =  distinct !{![[DO_DISABLE]],     ![[DISABLE]]}
+// CHECK: ![[DO_FULL]]       =  distinct !{![[DO_FULL]],        ![[FULL]]}

test/Parser/opencl-unroll-hint.cl

+//RUN: %clang_cc1 -O0 -cl-std=CL2.0 -fsyntax-only -verify %s
+
+kernel void B (global int *x) {
+  __attribute__((opencl_unroll_hint(42)))
+  if (x[0])                             // expected-error {{OpenCL only supports opencl_unroll_hint on for, while, and do statements}}
+    x[0] = 15;
+}
+

test/SemaOpenCL/unroll-hint.cl

+//RUN: %clang_cc1 -O0 -cl-std=CL2.0 -fsyntax-only -verify %s
+
+kernel void C (global int *x) {
+  int I = 3;
+  __attribute__((opencl_unroll_hint(I))) // expected-error {{opencl_unroll_hint attribute requires an integer constant}}
+  while (I--);
+}
+
+kernel void D (global int *x) {
+  int i = 10;
+  __attribute__((opencl_unroll_hint))
+  do {
+  } while(i--);
+}
+
+// ToDo: enable this test after fixing assertion
+#if 0 
+kernel void E() {
+  __attribute__((opencl_unroll_hint(2,4))) // expected-error {{opencl_unroll_hint attribute takes no more than 1 argument}}
+  for(int i=0; i<100; i++);
+}
+#endif