Treat opencl_unroll_hint subject errors as semantic rather than parse errors

[aaron.ballman]

Summary

While working on a downstream project, I noticed some issues with the opencl_unroll_hint implementation. The attribute definition claimed the attribute was inheritable (which only applies to declaration attributes) and not a statement attribute. Further, it treats subject appertainment errors as being parse errors rather than semantic errors, which leads to us accepting invalid code. For instance, we currently fail to reject:

void foo() {
  int i = 1000;
  __attribute__((nomerge, opencl_unroll_hint(8)))
  if (i) { foo(); }
}

This patch address the issues by clarifying that opencl_unroll_hint is a statement attribute and handling its appertainment checks in the semantic layer instead of the parsing layer. This changes the output of the diagnostic text to be more consistent with other appertainment errors.

[Anastasia]

LGTM! Thanks for fixing this.

[aaron.ballman]

Thanks for the review! I've commit in 45ccfd9c9d0311371a8217c15c2ef3ba969a0aff

clang/include/clang/Basic/Attr.td

 // the specification does not expose them with one currently.
 def OpenCLKernel : InheritableAttr {
   let Spellings = [Keyword<"__kernel">, Keyword<"kernel">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [Undocumented];
   let SimpleHandler = 1;
 }
 
-def OpenCLUnrollHint : InheritableAttr {
+def OpenCLUnrollHint : StmtAttr {
   let Spellings = [GNU<"opencl_unroll_hint">];
+//  let Subjects = SubjectList<[ForStmt, CXXForRangeStmt, WhileStmt, DoStmt],
+//                             ErrorDiag, "'for', 'while', and 'do' statements">;
   let Args = [UnsignedArgument<"UnrollHint">];
   let Documentation = [OpenCLUnrollHintDocs];
 }
 
 def OpenCLIntelReqdSubGroupSize: InheritableAttr {
   let Spellings = [GNU<"intel_reqd_sub_group_size">];
   let Args = [UnsignedArgument<"SubGroupSize">];
   let Subjects = SubjectList<[Function], ErrorDiag>;

clang/include/clang/Basic/DiagnosticParseKinds.td

 def err_pragma_attribute_expected_period : Error<
   "expected '.' after pragma attribute namespace %0">;
 def err_pragma_attribute_namespace_on_attribute : Error<
   "namespace can only apply to 'push' or 'pop' directives">;
 def note_pragma_attribute_namespace_on_attribute : Note<
   "omit the namespace to add attributes to the most-recently"
   " pushed attribute group">;
 
-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>;
 def warn_pragma_expected_predicate : Warning<
   "expected %select{'enable', 'disable', 'begin' or 'end'|'disable'}0 - ignoring">, InGroup<IgnoredPragmas>;
 def warn_pragma_begin_end_mismatch : Warning<
   "OpenCL extension end directive mismatches begin directive - ignoring">, InGroup<IgnoredPragmas>;
 def warn_pragma_unknown_extension : Warning<

clang/include/clang/Parse/Parser.h

                                   ParsedAttributes &Attrs);
   void ParseMicrosoftTypeAttributes(ParsedAttributes &attrs);
   void DiagnoseAndSkipExtendedMicrosoftTypeAttributes();
   SourceLocation SkipExtendedMicrosoftTypeAttributes();
   void ParseMicrosoftInheritanceClassAttributes(ParsedAttributes &attrs);
   void ParseBorlandTypeAttributes(ParsedAttributes &attrs);
   void ParseOpenCLKernelAttributes(ParsedAttributes &attrs);
   void ParseOpenCLQualifiers(ParsedAttributes &Attrs);
-  /// Parses opencl_unroll_hint attribute if language is OpenCL v2.0
-  /// or higher.
-  /// \return false if error happens.
-  bool MaybeParseOpenCLUnrollHintAttribute(ParsedAttributes &Attrs) {
-    if (getLangOpts().OpenCL)
-      return ParseOpenCLUnrollHintAttribute(Attrs);
-    return true;
-  }
-  /// Parses opencl_unroll_hint attribute.
-  /// \return false if error happens.
-  bool ParseOpenCLUnrollHintAttribute(ParsedAttributes &Attrs);
   void ParseNullabilityTypeSpecifiers(ParsedAttributes &attrs);
 
   VersionTuple ParseVersionTuple(SourceRange &Range);
   void ParseAvailabilityAttribute(IdentifierInfo &Availability,
                                   SourceLocation AvailabilityLoc,
                                   ParsedAttributes &attrs,
                                   SourceLocation *endLoc,
                                   IdentifierInfo *ScopeName,

clang/lib/Parse/ParseStmt.cpp

 ///
 StmtResult
 Parser::ParseStatementOrDeclaration(StmtVector &Stmts,
                                     ParsedStmtContext StmtCtx,
                                     SourceLocation *TrailingElseLoc) {
 
   ParenBraceBracketBalancer BalancerRAIIObj(*this);
 
+  // Because we're parsing either a statement or a declaration, the order of
+  // attribute parsing is important. [[]] attributes at the start of a
+  // statement are different from [[]] attributes that follow an __attribute__
+  // at the start of the statement. Thus, we're not using MaybeParseAttributes
+  // here because we don't want to allow arbitrary orderings.
   ParsedAttributesWithRange Attrs(AttrFactory);
   MaybeParseCXX11Attributes(Attrs, nullptr, /*MightBeObjCMessageSend*/ true);
-  if (!MaybeParseOpenCLUnrollHintAttribute(Attrs))
-    return StmtError();
+  if (getLangOpts().OpenCL)
+    MaybeParseGNUAttributes(Attrs);
 
   StmtResult Res = ParseStatementOrDeclarationAfterAttributes(
       Stmts, StmtCtx, TrailingElseLoc, Attrs);
   MaybeDestroyTemplateIds();
 
   assert((Attrs.empty() || Res.isInvalid() || Res.isUsable()) &&
          "attributes on empty statement");
 

   while (Tok.isNot(tok::r_brace)) {
     StmtResult R =
         ParseStatementOrDeclaration(Stmts, ParsedStmtContext::Compound);
     if (R.isUsable())
       Stmts.push_back(R.get());
   }
   Braces.consumeClose();
 }
-
-bool Parser::ParseOpenCLUnrollHintAttribute(ParsedAttributes &Attrs) {
-  MaybeParseGNUAttributes(Attrs);
-
-  if (Attrs.empty())
-    return true;
-
-  if (Attrs.begin()->getKind() != ParsedAttr::AT_OpenCLUnrollHint)
-    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;
-}

clang/lib/Sema/SemaStmtAttr.cpp

 static Attr *handleOpenCLUnrollHint(Sema &S, Stmt *St, const ParsedAttr &A,
                                     SourceRange Range) {
   // Although the feature was introduced only in OpenCL C v2.0 s6.11.5, it's
   // useful for OpenCL 1.x too and doesn't require HW support.
   // opencl_unroll_hint can have 0 arguments (compiler
   // determines unrolling factor) or 1 argument (the unroll factor provided
   // by the user).
 
+  if (!isa<ForStmt, CXXForRangeStmt, DoStmt, WhileStmt>(St)) {
+    S.Diag(A.getLoc(), diag::err_attribute_wrong_decl_type_str)
+        << A << "'for', 'while', and 'do' statements";
+    return nullptr;
+  }
+
   unsigned NumArgs = A.getNumArgs();
 
   if (NumArgs > 1) {
     S.Diag(A.getLoc(), diag::err_attribute_too_many_arguments) << A << 1;
     return nullptr;
   }
 
   unsigned UnrollFactor = 0;

clang/test/Parser/opencl-unroll-hint.cl

-//RUN: %clang_cc1 -O0 -cl-std=CL2.0 -fsyntax-only -verify %s
+//RUN: %clang_cc1 -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' attribute on for, while, and do statements}}
+  __attribute__((opencl_unroll_hint(42))) // expected-error {{'opencl_unroll_hint' attribute only applies to 'for', 'while', and 'do' statements}}
+  if (x[0])
     x[0] = 15;
 }
 
+void parse_order_error() {
+  // Ensure we properly diagnose OpenCL loop attributes on the incorrect
+  // subject in the presence of other attributes.
+  int i = 1000;
+  __attribute__((nomerge, opencl_unroll_hint(8))) // expected-error {{'opencl_unroll_hint' attribute only applies to 'for', 'while', and 'do' statements}}
+  if (i) { parse_order_error(); } // Recursive call silences unrelated diagnostic about nomerge.
+
+  __attribute__((opencl_unroll_hint(8), nomerge)) // expected-error {{'opencl_unroll_hint' attribute only applies to 'for', 'while', and 'do' statements}}
+  if (i) { parse_order_error(); } // Recursive call silences unrelated diagnostic about nomerge.
+
+  __attribute__((nomerge, opencl_unroll_hint(8))) // OK
+  while (1) { parse_order_error(); } // Recursive call silences unrelated diagnostic about nomerge.
+}