[SYCL] Remove a parsing error and make it a semantic error (#3158)

The SYCL loop attributes can only be applied to a for, do, or while
statement. However, we would diagnose this as a parsing error rather
than a semantic error. Attribute appertainment of [[]] attributes is
handled as a semantic error to avoid potentially changing the parsing
paths in the presence of an [[]] attribute.

As part of this change, I also corrected the attribute definitions to
properly reflect that these are statement attributes and have a
specific subject list. However, Clang's automated diagnostics focus
primarily on declaration attributes, so this change is mostly for
code readability rather than correctness (for the moment).

Author: AaronBallman

clang/include/clang/Basic/Attr.td

@@ -1383,8 +1383,10 @@ def OpenCLUnrollHint : InheritableAttr {
   let Documentation = [OpenCLUnrollHintDocs];
 }
 
-def LoopUnrollHint : InheritableAttr {
+def LoopUnrollHint : StmtAttr {
   let Spellings = [CXX11<"clang","loop_unroll">];
+  let Subjects = SubjectList<[ForStmt, CXXForRangeStmt, WhileStmt, DoStmt],
+                             ErrorDiag, "'for', 'while', and 'do' statements">;
   let Args = [ExprArgument<"UnrollHintExpr">];
   let LangOpts = [SYCLIsDevice, SYCLIsHost];
   let HasCustomTypeTransform = 1;
@@ -1802,9 +1804,11 @@ def Mode : Attr {
   let PragmaAttributeSupport = 0;
 }
 
-def SYCLIntelFPGAIVDep : Attr {
+def SYCLIntelFPGAIVDep : StmtAttr {
   let Spellings = [CXX11<"intelfpga","ivdep">,
                    CXX11<"intel","ivdep">];
+  let Subjects = SubjectList<[ForStmt, CXXForRangeStmt, WhileStmt, DoStmt],
+                             ErrorDiag, "'for', 'while', and 'do' statements">;
   let Args = [
     ExprArgument<"SafelenExpr">, ExprArgument<"ArrayExpr">,
     UnsignedArgument<"SafelenValue">
@@ -1850,58 +1854,72 @@ def SYCLIntelFPGAInitiationInterval : StmtAttr {
   let Spellings = [CXX11<"intelfpga","ii">,
                    CXX11<"intel","ii">,
                    CXX11<"intel", "initiation_interval">];
+  let Subjects = SubjectList<[ForStmt, CXXForRangeStmt, WhileStmt, DoStmt],
+                             ErrorDiag, "'for', 'while', and 'do' statements">;
   let Args = [ExprArgument<"IntervalExpr">];
   let LangOpts = [SYCLIsDevice, SYCLIsHost];
   let HasCustomTypeTransform = 1;
   let Documentation = [SYCLIntelFPGAInitiationIntervalAttrDocs];
 }
 
-def SYCLIntelFPGAMaxConcurrency : Attr {
+def SYCLIntelFPGAMaxConcurrency : StmtAttr {
   let Spellings = [CXX11<"intelfpga","max_concurrency">,
                    CXX11<"intel","max_concurrency">];
+  let Subjects = SubjectList<[ForStmt, CXXForRangeStmt, WhileStmt, DoStmt],
+                             ErrorDiag, "'for', 'while', and 'do' statements">;
   let Args = [ExprArgument<"NThreadsExpr">];
   let LangOpts = [SYCLIsDevice, SYCLIsHost];
   let HasCustomTypeTransform = 1;
   let Documentation = [SYCLIntelFPGAMaxConcurrencyAttrDocs];
 }
 
-def SYCLIntelFPGALoopCoalesce : Attr {
+def SYCLIntelFPGALoopCoalesce : StmtAttr {
   let Spellings = [CXX11<"intelfpga","loop_coalesce">,
                    CXX11<"intel","loop_coalesce">];
+  let Subjects = SubjectList<[ForStmt, CXXForRangeStmt, WhileStmt, DoStmt],
+                             ErrorDiag, "'for', 'while', and 'do' statements">;
   let Args = [ExprArgument<"NExpr">];
   let LangOpts = [SYCLIsDevice, SYCLIsHost];
   let HasCustomTypeTransform = 1;
   let Documentation = [SYCLIntelFPGALoopCoalesceAttrDocs];
 }
 
-def SYCLIntelFPGADisableLoopPipelining : Attr {
+def SYCLIntelFPGADisableLoopPipelining : StmtAttr {
   let Spellings = [CXX11<"intelfpga","disable_loop_pipelining">,
                    CXX11<"intel","disable_loop_pipelining">];
+  let Subjects = SubjectList<[ForStmt, CXXForRangeStmt, WhileStmt, DoStmt],
+                             ErrorDiag, "'for', 'while', and 'do' statements">;
   let LangOpts = [SYCLIsDevice, SYCLIsHost];
   let HasCustomTypeTransform = 1;
   let Documentation = [SYCLIntelFPGADisableLoopPipeliningAttrDocs];
 }
 
-def SYCLIntelFPGAMaxInterleaving : Attr {
+def SYCLIntelFPGAMaxInterleaving : StmtAttr {
   let Spellings = [CXX11<"intelfpga","max_interleaving">,
                    CXX11<"intel","max_interleaving">];
+  let Subjects = SubjectList<[ForStmt, CXXForRangeStmt, WhileStmt, DoStmt],
+                             ErrorDiag, "'for', 'while', and 'do' statements">;
   let Args = [ExprArgument<"NExpr">];
   let LangOpts = [SYCLIsDevice, SYCLIsHost];
   let HasCustomTypeTransform = 1;
   let Documentation = [SYCLIntelFPGAMaxInterleavingAttrDocs];
 }
 
-def SYCLIntelFPGASpeculatedIterations : Attr {
+def SYCLIntelFPGASpeculatedIterations : StmtAttr {
   let Spellings = [CXX11<"intelfpga","speculated_iterations">,
                    CXX11<"intel","speculated_iterations">];
+  let Subjects = SubjectList<[ForStmt, CXXForRangeStmt, WhileStmt, DoStmt],
+                             ErrorDiag, "'for', 'while', and 'do' statements">;
   let Args = [ExprArgument<"NExpr">];
   let LangOpts = [SYCLIsDevice, SYCLIsHost];
   let HasCustomTypeTransform = 1;
   let Documentation = [SYCLIntelFPGASpeculatedIterationsAttrDocs];
 }
 
-def SYCLIntelFPGANofusion : Attr {
+def SYCLIntelFPGANofusion : StmtAttr {
   let Spellings = [CXX11<"intel","nofusion">];
+  let Subjects = SubjectList<[ForStmt, CXXForRangeStmt, WhileStmt, DoStmt],
+                             ErrorDiag, "'for', 'while', and 'do' statements">;
   let LangOpts = [SYCLIsDevice, SYCLIsHost];
   let HasCustomTypeTransform = 1;
   let Documentation = [SYCLIntelFPGANofusionAttrDocs];

clang/include/clang/Basic/DiagnosticParseKinds.td

@@ -1433,12 +1433,6 @@ def err_pragma_cannot_end_force_cuda_host_device : Error<
   "force_cuda_host_device begin">;
 } // end of Parse Issue category.
 
-// SYCL loop pragmas and attributes support
-// * Intel FPGA attribute ivdep, ii, max_concurrency support
-// * clang::loop_unroll attribute support
-def err_loop_attr_on_non_loop : Error<
-  "%select{clang|intelfpga}0 loop attributes must be applied to for, while, or do statements">;
-
 let CategoryName = "Modules Issue" in {
 def err_unexpected_module_decl : Error<
   "module declaration can only appear at the top level">;

clang/include/clang/Parse/Parser.h

@@ -2831,14 +2831,6 @@ class Parser : public CodeCompletionHandler {
   /// \return false if error happens.
   bool ParseOpenCLUnrollHintAttribute(ParsedAttributes &Attrs);
 
-  /// Parses intelfpga:: and clang:: loop attributes if the language is SYCL
-  bool MaybeParseSYCLLoopAttributes(ParsedAttributes &Attrs) {
-    if (getLangOpts().SYCLIsDevice || getLangOpts().SYCLIsHost)
-      return ParseSYCLLoopAttributes(Attrs);
-    return true;
-  }
-  bool ParseSYCLLoopAttributes(ParsedAttributes &Attrs);
-
   void ParseNullabilityTypeSpecifiers(ParsedAttributes &attrs);
   VersionTuple ParseVersionTuple(SourceRange &Range);
   void ParseAvailabilityAttribute(IdentifierInfo &Availability,

clang/lib/Parse/ParseStmt.cpp

@@ -100,8 +100,7 @@ Parser::ParseStatementOrDeclaration(StmtVector &Stmts,
 
   ParsedAttributesWithRange Attrs(AttrFactory);
   MaybeParseCXX11Attributes(Attrs, nullptr, /*MightBeObjCMessageSend*/ true);
-  if (!MaybeParseOpenCLUnrollHintAttribute(Attrs) ||
-      !MaybeParseSYCLLoopAttributes(Attrs))
+  if (!MaybeParseOpenCLUnrollHintAttribute(Attrs))
     return StmtError();
 
   StmtResult Res = ParseStatementOrDeclarationAfterAttributes(
@@ -2565,32 +2564,3 @@ bool Parser::ParseOpenCLUnrollHintAttribute(ParsedAttributes &Attrs) {
   }
   return true;
 }
-
-bool Parser::ParseSYCLLoopAttributes(ParsedAttributes &Attrs) {
-  MaybeParseCXX11Attributes(Attrs);
-
-  if (Attrs.empty())
-    return true;
-
-  if (Attrs.begin()->getKind() != ParsedAttr::AT_SYCLIntelFPGAIVDep &&
-      Attrs.begin()->getKind() !=
-          ParsedAttr::AT_SYCLIntelFPGAInitiationInterval &&
-      Attrs.begin()->getKind() != ParsedAttr::AT_SYCLIntelFPGAMaxConcurrency &&
-      Attrs.begin()->getKind() != ParsedAttr::AT_SYCLIntelFPGALoopCoalesce &&
-      Attrs.begin()->getKind() !=
-          ParsedAttr::AT_SYCLIntelFPGADisableLoopPipelining &&
-      Attrs.begin()->getKind() != ParsedAttr::AT_SYCLIntelFPGAMaxInterleaving &&
-      Attrs.begin()->getKind() !=
-          ParsedAttr::AT_SYCLIntelFPGASpeculatedIterations &&
-      Attrs.begin()->getKind() != ParsedAttr::AT_LoopUnrollHint &&
-      Attrs.begin()->getKind() != ParsedAttr::AT_SYCLIntelFPGANofusion)
-    return true;
-
-  bool IsIntelFPGAAttribute = (Attrs.begin()->getKind() != ParsedAttr::AT_LoopUnrollHint);
-
-  if (!(Tok.is(tok::kw_for) || Tok.is(tok::kw_while) || Tok.is(tok::kw_do))) {
-    Diag(Tok, diag::err_loop_attr_on_non_loop) << IsIntelFPGAAttribute;
-    return false;
-  }
-  return true;
-}

clang/lib/Sema/SemaStmtAttr.cpp

@@ -76,10 +76,16 @@ static Attr *handleSuppressAttr(Sema &S, Stmt *St, const ParsedAttr &A,
 }
 
 template <typename FPGALoopAttrT>
-static Attr *handleIntelFPGALoopAttr(Sema &S, const ParsedAttr &A) {
+static Attr *handleIntelFPGALoopAttr(Sema &S, Stmt *St, const ParsedAttr &A) {
   if(S.LangOpts.SYCLIsHost)
     return nullptr;
 
+  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::warn_attribute_too_many_arguments) << A << 1;
@@ -104,10 +110,16 @@ static Attr *handleIntelFPGALoopAttr(Sema &S, const ParsedAttr &A) {
 
 template <>
 Attr *handleIntelFPGALoopAttr<SYCLIntelFPGADisableLoopPipeliningAttr>(
-    Sema &S, const ParsedAttr &A) {
+    Sema &S, Stmt *St, const ParsedAttr &A) {
   if (S.LangOpts.SYCLIsHost)
     return nullptr;
 
+  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 > 0) {
     S.Diag(A.getLoc(), diag::warn_attribute_too_many_arguments) << A << 0;
@@ -270,7 +282,13 @@ CheckRedundantSYCLIntelFPGAIVDepAttrs(Sema &S, ArrayRef<const Attr *> Attrs) {
   }
 }
 
-static Attr *handleIntelFPGAIVDepAttr(Sema &S, const ParsedAttr &A) {
+static Attr *handleIntelFPGAIVDepAttr(Sema &S, Stmt *St, const ParsedAttr &A) {
+  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 > 2) {
     S.Diag(A.getLoc(), diag::err_attribute_too_many_arguments) << A << 2;
@@ -284,10 +302,17 @@ static Attr *handleIntelFPGAIVDepAttr(Sema &S, const ParsedAttr &A) {
       NumArgs == 2 ? A.getArgAsExpr(1) : nullptr);
 }
 
-static Attr *handleIntelFPGANofusionAttr(Sema &S, const ParsedAttr &A) {
+static Attr *handleIntelFPGANofusionAttr(Sema &S, Stmt *St,
+                                         const ParsedAttr &A) {
   if (S.LangOpts.SYCLIsHost)
     return nullptr;
 
+  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 > 0) {
     S.Diag(A.getLoc(), diag::warn_attribute_too_many_arguments) << A << 0;
@@ -751,8 +776,17 @@ static Attr *handleLoopUnrollHint(Sema &S, Stmt *St, const ParsedAttr &A,
   Expr *E = NumArgs ? A.getArgAsExpr(0) : nullptr;
   if (A.getParsedKind() == ParsedAttr::AT_OpenCLUnrollHint)
     return S.BuildOpenCLLoopUnrollHintAttr(A, E);
-  else if (A.getParsedKind() == ParsedAttr::AT_LoopUnrollHint)
+  else if (A.getParsedKind() == ParsedAttr::AT_LoopUnrollHint) {
+    // FIXME: this should be hoisted up to the top level, but can't be placed
+    // there until the opencl attribute has its parsing error converted into a
+    // semantic error. See: Parser::ParseOpenCLUnrollHintAttribute().
+    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;
+    }
     return S.BuildLoopUnrollHintAttr(A, E);
+  }
 
   return nullptr;
 }
@@ -771,20 +805,22 @@ static Attr *ProcessStmtAttribute(Sema &S, Stmt *St, const ParsedAttr &A,
   case ParsedAttr::AT_LoopHint:
     return handleLoopHintAttr(S, St, A, Range);
   case ParsedAttr::AT_SYCLIntelFPGAIVDep:
-    return handleIntelFPGAIVDepAttr(S, A);
+    return handleIntelFPGAIVDepAttr(S, St, A);
   case ParsedAttr::AT_SYCLIntelFPGAInitiationInterval:
-    return handleIntelFPGALoopAttr<SYCLIntelFPGAInitiationIntervalAttr>(S, A);
+    return handleIntelFPGALoopAttr<SYCLIntelFPGAInitiationIntervalAttr>(S, St,
+                                                                        A);
   case ParsedAttr::AT_SYCLIntelFPGAMaxConcurrency:
-    return handleIntelFPGALoopAttr<SYCLIntelFPGAMaxConcurrencyAttr>(S, A);
+    return handleIntelFPGALoopAttr<SYCLIntelFPGAMaxConcurrencyAttr>(S, St, A);
   case ParsedAttr::AT_SYCLIntelFPGALoopCoalesce:
-    return handleIntelFPGALoopAttr<SYCLIntelFPGALoopCoalesceAttr>(S, A);
+    return handleIntelFPGALoopAttr<SYCLIntelFPGALoopCoalesceAttr>(S, St, A);
   case ParsedAttr::AT_SYCLIntelFPGADisableLoopPipelining:
-    return handleIntelFPGALoopAttr<SYCLIntelFPGADisableLoopPipeliningAttr>(S,
-                                                                           A);
+    return handleIntelFPGALoopAttr<SYCLIntelFPGADisableLoopPipeliningAttr>(
+        S, St, A);
   case ParsedAttr::AT_SYCLIntelFPGAMaxInterleaving:
-    return handleIntelFPGALoopAttr<SYCLIntelFPGAMaxInterleavingAttr>(S, A);
+    return handleIntelFPGALoopAttr<SYCLIntelFPGAMaxInterleavingAttr>(S, St, A);
   case ParsedAttr::AT_SYCLIntelFPGASpeculatedIterations:
-    return handleIntelFPGALoopAttr<SYCLIntelFPGASpeculatedIterationsAttr>(S, A);
+    return handleIntelFPGALoopAttr<SYCLIntelFPGASpeculatedIterationsAttr>(S, St,
+                                                                          A);
   case ParsedAttr::AT_OpenCLUnrollHint:
   case ParsedAttr::AT_LoopUnrollHint:
     return handleLoopUnrollHint(S, St, A, Range);
@@ -797,7 +833,7 @@ static Attr *ProcessStmtAttribute(Sema &S, Stmt *St, const ParsedAttr &A,
   case ParsedAttr::AT_Unlikely:
     return handleUnlikely(S, St, A, Range);
   case ParsedAttr::AT_SYCLIntelFPGANofusion:
-    return handleIntelFPGANofusionAttr(S, A);
+    return handleIntelFPGANofusionAttr(S, St, A);
   default:
     // if we're here, then we parsed a known attribute, but didn't recognize
     // it as a statement attribute => it is declaration attribute

clang/test/SemaSYCL/intel-fpga-loops.cpp

@@ -2,30 +2,21 @@
 
 // Test for Intel FPGA loop attributes applied not to a loop
 void foo() {
-  // expected-error@+1 {{intelfpga loop attributes must be applied to for, while, or do statements}}
+  // expected-error@+1 {{'ivdep' attribute only applies to 'for', 'while', and 'do' statements}}
   [[intel::ivdep]] int a[10];
-  // expected-error@+1 {{intelfpga loop attributes must be applied to for, while, or do statements}}
-  [[intel::ivdep(2)]] int b[10];
-  // expected-error@+1 {{intelfpga loop attributes must be applied to for, while, or do statements}}
+  // expected-error@+1 {{'initiation_interval' attribute only applies to 'for', 'while', and 'do' statements}}
   [[intel::initiation_interval(2)]] int c[10];
-  // expected-error@+1 {{intelfpga loop attributes must be applied to for, while, or do statements}}
+  // expected-error@+1 {{'max_concurrency' attribute only applies to 'for', 'while', and 'do' statements}}
   [[intel::max_concurrency(2)]] int d[10];
-
-  int arr[10];
-  // expected-error@+1 {{intelfpga loop attributes must be applied to for, while, or do statements}}
-  [[intel::ivdep(arr)]] int e[10];
-  // expected-error@+1 {{intelfpga loop attributes must be applied to for, while, or do statements}}
-  [[intel::ivdep(arr, 2)]] int f[10];
-
-  // expected-error@+1 {{intelfpga loop attributes must be applied to for, while, or do statements}}
+  // expected-error@+1 {{'disable_loop_pipelining' attribute only applies to 'for', 'while', and 'do' statements}}
   [[intel::disable_loop_pipelining]] int g[10];
-  // expected-error@+1 {{intelfpga loop attributes must be applied to for, while, or do statements}}
+  // expected-error@+1 {{'loop_coalesce' attribute only applies to 'for', 'while', and 'do' statements}}
   [[intel::loop_coalesce(2)]] int h[10];
-  // expected-error@+1 {{intelfpga loop attributes must be applied to for, while, or do statements}}
+  // expected-error@+1 {{'max_interleaving' attribute only applies to 'for', 'while', and 'do' statements}}
   [[intel::max_interleaving(4)]] int i[10];
-  // expected-error@+1 {{intelfpga loop attributes must be applied to for, while, or do statements}}
+  // expected-error@+1 {{'speculated_iterations' attribute only applies to 'for', 'while', and 'do' statements}}
   [[intel::speculated_iterations(6)]] int j[10];
-  // expected-error@+1 {{intelfpga loop attributes must be applied to for, while, or do statements}}
+  // expected-error@+1 {{'nofusion' attribute only applies to 'for', 'while', and 'do' statements}}
   [[intel::nofusion]] int k[10];
 }
 
@@ -426,3 +417,14 @@ int main() {
   });
   return 0;
 }
+
+void parse_order_error() {
+  // We had a bug where we would only look at the first attribute in the group
+  // when trying to determine whether to diagnose the loop attributes on an
+  // incorrect subject. Test that we properly catch this situation.
+  [[clang::nomerge, intel::max_concurrency(1)]] // expected-error {{'max_concurrency' attribute only applies to 'for', 'while', and 'do' statements}}
+  if (1) { parse_order_error(); }               // Recursive call silences unrelated diagnostic about nomerge.
+
+  [[clang::nomerge, intel::max_concurrency(1)]] // OK
+  while (1) { parse_order_error(); }            // Recursive call silences unrelated diagnostic about nomerge.
+}

clang/test/SemaSYCL/loop_unroll.cpp

@@ -8,7 +8,7 @@ void bar() {
 }
 
 void foo() {
-  // expected-error@+1 {{clang loop attributes must be applied to for, while, or do statements}}
+  // expected-error@+1 {{'loop_unroll' attribute only applies to 'for', 'while', and 'do' statements}}
   [[clang::loop_unroll(8)]] int a[10];
 
   // expected-error@+1 {{'loop_unroll' attribute takes no more than 1 argument}}

clang/utils/TableGen/ClangAttrEmitter.cpp

@@ -3526,6 +3526,9 @@ static void GenerateCustomAppertainsTo(const Record &Subject, raw_ostream &OS) {
 }
 
 static void GenerateAppertainsTo(const Record &Attr, raw_ostream &OS) {
+  // FIXME: this function only diagnoses declaration attributes and lacks
+  // reasonable support for statement attributes.
+
   // If the attribute does not contain a Subjects definition, then use the
   // default appertainsTo logic.
   if (Attr.isValueUnset("Subjects"))
@@ -3557,7 +3560,16 @@ static void GenerateAppertainsTo(const Record &Attr, raw_ostream &OS) {
     // because it requires the subject to be of a specific type, and were that
     // information inlined here, it would not support an attribute with multiple
     // custom subjects.
-    if ((*I)->isSubClassOf("SubsetSubject")) {
+    //
+    // If the subject is a statement rather than a declaration node, use 'true'
+    // as the test so that statement nodes can be mixed with declaration nodes
+    // for attributes which appertain to both statements and declarations. If
+    // all of the nodes are statement nodes (so the diagnostic predicate is
+    // trivially true), this will diagnose the use of the attribute on any
+    // declaration, which is reasonable.
+    if ((*I)->isSubClassOf("StmtNode")) {
+      OS << "true";
+    } else if ((*I)->isSubClassOf("SubsetSubject")) {
       OS << "!" << functionNameForCustomAppertainsTo(**I) << "(D)";
     } else {
       OS << "!isa<" << GetSubjectWithSuffix(*I) << ">(D)";