[SYCL] Correct the tablegen for checking mutually exclusive stmt attrs (#3519)

The work that was done upstream turned out to be insufficient for
checking statement attribute mutual exclusion because attributed
statements do not collect their attributes one-at-a-time in the same
way that declarations do. So the design that was attempting to check
for mutual exclusion as each attribute was processed would not ever
catch a mutual exclusion.

The new approach is to check all of attributes that are to be applied
to the attributed statement in a group. This required generating
another DiagnoseMutualExclusions() function into AttrParsedAttrImpl.inc.

Author: AaronBallman

clang/include/clang/Basic/Attr.td

@@ -1952,6 +1952,10 @@ def SYCLIntelFPGADisableLoopPipelining : DeclOrStmtAttr {
 }
 def : MutualExclusions<[SYCLIntelFPGAInitiationInterval,
                         SYCLIntelFPGADisableLoopPipelining]>;
+def : MutualExclusions<[SYCLIntelFPGAIVDep,
+                        SYCLIntelFPGADisableLoopPipelining]>;
+def : MutualExclusions<[SYCLIntelFPGAMaxConcurrency,
+                        SYCLIntelFPGADisableLoopPipelining]>;
 
 def SYCLIntelFPGAMaxInterleaving : StmtAttr {
   let Spellings = [CXX11<"intelfpga","max_interleaving">,
@@ -1963,6 +1967,8 @@ def SYCLIntelFPGAMaxInterleaving : StmtAttr {
   let HasCustomTypeTransform = 1;
   let Documentation = [SYCLIntelFPGAMaxInterleavingAttrDocs];
 }
+def : MutualExclusions<[SYCLIntelFPGADisableLoopPipelining,
+                        SYCLIntelFPGAMaxInterleaving]>;
 
 def SYCLIntelFPGASpeculatedIterations : StmtAttr {
   let Spellings = [CXX11<"intelfpga","speculated_iterations">,
@@ -1974,6 +1980,8 @@ def SYCLIntelFPGASpeculatedIterations : StmtAttr {
   let HasCustomTypeTransform = 1;
   let Documentation = [SYCLIntelFPGASpeculatedIterationsAttrDocs];
 }
+def : MutualExclusions<[SYCLIntelFPGADisableLoopPipelining,
+                        SYCLIntelFPGASpeculatedIterations]>;
 
 def SYCLIntelFPGANofusion : StmtAttr {
   let Spellings = [CXX11<"intel","nofusion">];

clang/include/clang/Sema/ParsedAttr.h

@@ -95,12 +95,6 @@ struct ParsedAttrInfo {
                                    const Decl *D) const {
     return true;
   }
-  /// Check if the given attribute is mutually exclusive with other attributes
-  /// already applied to the given statement.
-  virtual bool diagMutualExclusion(Sema &S, const ParsedAttr &A,
-                                   const Stmt *St) const {
-    return true;
-  }
   /// Check if this attribute is allowed by the language we are compiling, and
   /// issue a diagnostic if not.
   virtual bool diagLangOpts(Sema &S, const ParsedAttr &Attr) const {
@@ -615,7 +609,12 @@ class ParsedAttr final
   bool diagnoseAppertainsTo(class Sema &S, const Decl *D) const;
   bool diagnoseAppertainsTo(class Sema &S, const Stmt *St) const;
   bool diagnoseMutualExclusion(class Sema &S, const Decl *D) const;
-  bool diagnoseMutualExclusion(class Sema &S, const Stmt *St) const;
+  // This function stub exists for parity with the declaration checking code so
+  // that checkCommonAttributeFeatures() can work generically on declarations
+  // or statements.
+  bool diagnoseMutualExclusion(class Sema &S, const Stmt *St) const {
+    return true;
+  }
   bool appliesToDecl(const Decl *D, attr::SubjectMatchRule MatchRule) const;
   void getMatchRules(const LangOptions &LangOpts,
                      SmallVectorImpl<std::pair<attr::SubjectMatchRule, bool>>

clang/lib/Sema/ParsedAttr.cpp

@@ -167,10 +167,6 @@ bool ParsedAttr::diagnoseMutualExclusion(Sema &S, const Decl *D) const {
   return getInfo().diagMutualExclusion(S, *this, D);
 }
 
-bool ParsedAttr::diagnoseMutualExclusion(Sema &S, const Stmt *St) const {
-  return getInfo().diagMutualExclusion(S, *this, St);
-}
-
 bool ParsedAttr::appliesToDecl(const Decl *D,
                                attr::SubjectMatchRule MatchRule) const {
   return checkAttributeMatchRuleAppliesTo(D, MatchRule);

clang/lib/Sema/SemaDecl.cpp

@@ -2540,9 +2540,9 @@ static bool mergeAlignedAttrs(Sema &S, NamedDecl *New, Decl *Old) {
   return AnyAdded;
 }
 
-#define WANT_MERGE_LOGIC
+#define WANT_DECL_MERGE_LOGIC
 #include "clang/Sema/AttrParsedAttrImpl.inc"
-#undef WANT_MERGE_LOGIC
+#undef WANT_DECL_MERGE_LOGIC
 
 static bool mergeDeclAttribute(Sema &S, NamedDecl *D,
                                const InheritableAttr *Attr,

clang/lib/Sema/SemaStmtAttr.cpp

@@ -407,9 +407,22 @@ static Attr *handleUnlikely(Sema &S, Stmt *St, const ParsedAttr &A,
   return ::new (S.Context) UnlikelyAttr(S.Context, A);
 }
 
+#define WANT_STMT_MERGE_LOGIC
+#include "clang/Sema/AttrParsedAttrImpl.inc"
+#undef WANT_STMT_MERGE_LOGIC
+
 static void
 CheckForIncompatibleAttributes(Sema &S,
                                const SmallVectorImpl<const Attr *> &Attrs) {
+  // The vast majority of attributed statements will only have one attribute
+  // on them, so skip all of the checking in the common case.
+  if (Attrs.size() < 2)
+    return;
+
+  // First, check for the easy cases that are table-generated for us.
+  if (!DiagnoseMutualExclusions(S, Attrs))
+    return;
+
   // There are 6 categories of loop hints attributes: vectorize, interleave,
   // unroll, unroll_and_jam, pipeline and distribute. Except for distribute they
   // come in two variants: a state form and a numeric form.  The state form
@@ -532,33 +545,6 @@ CheckForDuplicationSYCLLoopAttribute(Sema &S,
   }
 }
 
-/// Diagnose mutually exclusive attributes when present on a given
-/// declaration. Returns true if diagnosed.
-template <typename LoopAttrT, typename LoopAttrT2>
-static void CheckMutualExclusionSYCLLoopAttribute(
-    Sema &S, const SmallVectorImpl<const Attr *> &Attrs) {
-  std::pair<bool, bool> SeenAttrs;
-  const Attr *FirstSeen = nullptr;
-
-  for (const auto *I : Attrs) {
-    // Remember the first attribute of the problematic type so that we can
-    // potentially diagnose it later.
-    if (!FirstSeen && isa<LoopAttrT, LoopAttrT2>(I))
-      FirstSeen = I;
-
-    // Remember if we've seen either of the attribute types.
-    if (isa<LoopAttrT>(I))
-      SeenAttrs.first = true;
-    else if (isa<LoopAttrT2>(I))
-      SeenAttrs.second = true;
-
-    // If we've seen both of the attribute types, then diagnose them both.
-    if (SeenAttrs.first && SeenAttrs.second)
-      S.Diag(I->getLocation(), diag::err_attributes_are_not_compatible)
-          << FirstSeen << I;
-  }
-}
-
 static void CheckForIncompatibleSYCLLoopAttributes(
     Sema &S, const SmallVectorImpl<const Attr *> &Attrs) {
   CheckForDuplicationSYCLLoopAttribute<SYCLIntelFPGAInitiationIntervalAttr>(
@@ -573,21 +559,6 @@ static void CheckForIncompatibleSYCLLoopAttributes(
   CheckForDuplicationSYCLLoopAttribute<SYCLIntelFPGASpeculatedIterationsAttr>(
       S, Attrs);
   CheckForDuplicationSYCLLoopAttribute<LoopUnrollHintAttr>(S, Attrs, false);
-  CheckMutualExclusionSYCLLoopAttribute<SYCLIntelFPGADisableLoopPipeliningAttr,
-                                        SYCLIntelFPGAMaxInterleavingAttr>(
-      S, Attrs);
-  CheckMutualExclusionSYCLLoopAttribute<SYCLIntelFPGADisableLoopPipeliningAttr,
-                                        SYCLIntelFPGASpeculatedIterationsAttr>(
-      S, Attrs);
-  CheckMutualExclusionSYCLLoopAttribute<SYCLIntelFPGADisableLoopPipeliningAttr,
-                                        SYCLIntelFPGAInitiationIntervalAttr>(
-      S, Attrs);
-  CheckMutualExclusionSYCLLoopAttribute<SYCLIntelFPGADisableLoopPipeliningAttr,
-                                        SYCLIntelFPGAIVDepAttr>(S, Attrs);
-  CheckMutualExclusionSYCLLoopAttribute<SYCLIntelFPGADisableLoopPipeliningAttr,
-                                        SYCLIntelFPGAMaxConcurrencyAttr>(S,
-                                                                         Attrs);
-
   CheckRedundantSYCLIntelFPGAIVDepAttrs(S, Attrs);
   CheckForDuplicationSYCLLoopAttribute<SYCLIntelFPGANofusionAttr>(S, Attrs);
 }

clang/test/SemaCXX/attr-likelihood.cpp

@@ -147,5 +147,11 @@ void o()
   if constexpr (true) [[likely]] {
   // expected-warning@+1 {{attribute 'likely' has no effect when annotating an 'if constexpr' statement}}
   } else [[likely]];
+
+  if (1) [[likely, unlikely]] { // expected-error {{'unlikely' and 'likely' attributes are not compatible}} \
+                                // expected-note {{conflicting attribute is here}}
+  } else [[unlikely]][[likely]] { // expected-error {{'likely' and 'unlikely' attributes are not compatible}} \
+                                  // expected-note {{conflicting attribute is here}}
+  }
 }
 #endif

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

@@ -313,23 +313,28 @@ void loop_attrs_compatibility() {
   [[intel::disable_loop_pipelining]]
   [[intel::loop_coalesce]] for (int i = 0; i != 10; ++i)
       a[i] = 0;
-  // expected-error@+2 {{'disable_loop_pipelining' and 'max_interleaving' attributes are not compatible}}
+  // expected-error@+3 {{'max_interleaving' and 'disable_loop_pipelining' attributes are not compatible}}
+  // expected-note@+1 {{conflicting attribute is here}}
   [[intel::disable_loop_pipelining]]
   [[intel::max_interleaving(0)]] for (int i = 0; i != 10; ++i)
       a[i] = 0;
-  // expected-error@+2 {{'speculated_iterations' and 'disable_loop_pipelining' attributes are not compatible}}
+  // expected-error@+3 {{'disable_loop_pipelining' and 'speculated_iterations' attributes are not compatible}}
+  // expected-note@+1 {{conflicting attribute is here}}
   [[intel::speculated_iterations(0)]]
   [[intel::disable_loop_pipelining]] for (int i = 0; i != 10; ++i)
       a[i] = 0;
-  // expected-error@+2 {{'disable_loop_pipelining' and 'max_concurrency' attributes are not compatible}}
+  // expected-error@+3 {{'max_concurrency' and 'disable_loop_pipelining' attributes are not compatible}}
+  // expected-note@+1 {{conflicting attribute is here}}
   [[intel::disable_loop_pipelining]]
   [[intel::max_concurrency(0)]] for (int i = 0; i != 10; ++i)
       a[i] = 0;
-  // expected-error@+2 {{'initiation_interval' and 'disable_loop_pipelining' attributes are not compatible}}
+  // expected-error@+3 {{'disable_loop_pipelining' and 'initiation_interval' attributes are not compatible}}
+  // expected-note@+1 {{conflicting attribute is here}}
   [[intel::initiation_interval(10)]]
   [[intel::disable_loop_pipelining]] for (int i = 0; i != 10; ++i)
       a[i] = 0;
-  // expected-error@+2 {{'disable_loop_pipelining' and 'ivdep' attributes are not compatible}}
+  // expected-error@+3 {{'ivdep' and 'disable_loop_pipelining' attributes are not compatible}}
+  // expected-note@+1 {{conflicting attribute is here}}
   [[intel::disable_loop_pipelining]]
   [[intel::ivdep]] for (int i = 0; i != 10; ++i)
       a[i] = 0;

clang/utils/TableGen/ClangAttrEmitter.cpp

@@ -3653,7 +3653,8 @@ static void GenerateAppertainsTo(const Record &Attr, raw_ostream &OS) {
 static void GenerateMutualExclusionsChecks(const Record &Attr,
                                            const RecordKeeper &Records,
                                            raw_ostream &OS,
-                                           raw_ostream &MergeOS) {
+                                           raw_ostream &MergeDeclOS,
+                                           raw_ostream &MergeStmtOS) {
   // Find all of the definitions that inherit from MutualExclusions and include
   // the given attribute in the list of exclusions to generate the
   // diagMutualExclusion() check.
@@ -3717,43 +3718,59 @@ static void GenerateMutualExclusionsChecks(const Record &Attr,
     // Sema &S, Decl *D, Attr *A and that returns a bool (false on diagnostic,
     // true on success).
     if (Attr.isSubClassOf("InheritableAttr")) {
-      MergeOS << "  if (const auto *Second = dyn_cast<"
-              << (Attr.getName() + "Attr").str() << ">(A)) {\n";
+      MergeDeclOS << "  if (const auto *Second = dyn_cast<"
+                  << (Attr.getName() + "Attr").str() << ">(A)) {\n";
       for (const std::string &A : DeclAttrs) {
-        MergeOS << "    if (const auto *First = D->getAttr<" << A << ">()) {\n";
-        MergeOS << "      S.Diag(First->getLocation(), "
-                << "diag::err_attributes_are_not_compatible) << First << "
-                << "Second;\n";
-        MergeOS << "      S.Diag(Second->getLocation(), "
-                << "diag::note_conflicting_attribute);\n";
-        MergeOS << "      return false;\n";
-        MergeOS << "    }\n";
+        MergeDeclOS << "    if (const auto *First = D->getAttr<" << A
+                    << ">()) {\n";
+        MergeDeclOS << "      S.Diag(First->getLocation(), "
+                    << "diag::err_attributes_are_not_compatible) << First << "
+                    << "Second;\n";
+        MergeDeclOS << "      S.Diag(Second->getLocation(), "
+                    << "diag::note_conflicting_attribute);\n";
+        MergeDeclOS << "      return false;\n";
+        MergeDeclOS << "    }\n";
       }
-      MergeOS << "    return true;\n";
-      MergeOS << "  }\n";
+      MergeDeclOS << "    return true;\n";
+      MergeDeclOS << "  }\n";
     }
   }
+
+  // Statement attributes are a bit different from declarations. With
+  // declarations, each attribute is added to the declaration as it is
+  // processed, and so you can look on the Decl * itself to see if there is a
+  // conflicting attribute. Statement attributes are processed as a group
+  // because AttributedStmt needs to tail-allocate all of the attribute nodes
+  // at once. This means we cannot check whether the statement already contains
+  // an attribute to check for the conflict. Instead, we need to check whether
+  // the given list of semantic attributes contain any conflicts. It is assumed
+  // this code will be executed in the context of a function with parameters:
+  // Sema &S, const SmallVectorImpl<const Attr *> &C. The code will be within a
+  // loop which loops over the container C with a loop variable named A to
+  // represent the current attribute to check for conflicts.
+  //
+  // FIXME: it would be nice not to walk over the list of potential attributes
+  // to apply to the statement more than once, but statements typically don't
+  // have long lists of attributes on them, so re-walking the list should not
+  // be an expensive operation.
   if (!StmtAttrs.empty()) {
-    // Generate the ParsedAttrInfo subclass logic for statements.
-    OS << "  bool diagMutualExclusion(Sema &S, const ParsedAttr &AL, "
-       << "const Stmt *St) const override {\n";
-    OS << "    if (const auto *AS = dyn_cast<AttributedStmt>(St)) {\n";
-    OS << "      const ArrayRef<const Attr *> &Attrs = AS->getAttrs();\n";
-    for (const std::string &A : StmtAttrs) {
-      OS << "      auto Iter" << A << " = llvm::find_if(Attrs, [](const Attr "
-         << "*A) { return isa<" << A << ">(A); });\n";
-      OS << "      if (Iter" << A << " != Attrs.end()) {\n";
-      OS << "        S.Diag(AL.getLoc(), "
-         << "diag::err_attributes_are_not_compatible) << AL << *Iter" << A
-         << ";\n";
-      OS << "        S.Diag((*Iter" << A << ")->getLocation(), "
-         << "diag::note_conflicting_attribute);\n";
-      OS << "        return false;\n";
-      OS << "      }\n";
-    }
-    OS << "    }\n";
-    OS << "    return true;\n";
-    OS << "  }\n\n";
+    MergeStmtOS << "    if (const auto *Second = dyn_cast<"
+                << (Attr.getName() + "Attr").str() << ">(A)) {\n";
+    MergeStmtOS << "      auto Iter = llvm::find_if(C, [](const Attr *Check) "
+                << "{ return isa<";
+    interleave(
+        StmtAttrs, [&](const std::string &Name) { MergeStmtOS << Name; },
+        [&] { MergeStmtOS << ", "; });
+    MergeStmtOS << ">(Check); });\n";
+    MergeStmtOS << "      if (Iter != C.end()) {\n";
+    MergeStmtOS << "        S.Diag((*Iter)->getLocation(), "
+                << "diag::err_attributes_are_not_compatible) << *Iter << "
+                << "Second;\n";
+    MergeStmtOS << "        S.Diag(Second->getLocation(), "
+                << "diag::note_conflicting_attribute);\n";
+    MergeStmtOS << "        return false;\n";
+    MergeStmtOS << "      }\n";
+    MergeStmtOS << "    }\n";
   }
 }
 
@@ -3905,7 +3922,8 @@ static bool IsKnownToGCC(const Record &Attr) {
 void EmitClangAttrParsedAttrImpl(RecordKeeper &Records, raw_ostream &OS) {
   emitSourceFileHeader("Parsed attribute helpers", OS);
 
-  OS << "#if !defined(WANT_MERGE_LOGIC)\n";
+  OS << "#if !defined(WANT_DECL_MERGE_LOGIC) && "
+     << "!defined(WANT_STMT_MERGE_LOGIC)\n";
   PragmaClangAttributeSupport &PragmaAttributeSupport =
       getPragmaAttributeSupport(Records);
 
@@ -3929,8 +3947,8 @@ void EmitClangAttrParsedAttrImpl(RecordKeeper &Records, raw_ostream &OS) {
   // This stream is used to collect all of the declaration attribute merging
   // logic for performing mutual exclusion checks. This gets emitted at the
   // end of the file in a helper function of its own.
-  std::string DeclMergeChecks;
-  raw_string_ostream MergeOS(DeclMergeChecks);
+  std::string DeclMergeChecks, StmtMergeChecks;
+  raw_string_ostream MergeDeclOS(DeclMergeChecks), MergeStmtOS(StmtMergeChecks);
 
   // Generate a ParsedAttrInfo struct for each of the attributes.
   for (auto I = Attrs.begin(), E = Attrs.end(); I != E; ++I) {
@@ -3987,7 +4005,7 @@ void EmitClangAttrParsedAttrImpl(RecordKeeper &Records, raw_ostream &OS) {
       OS << "    Spellings = " << I->first << "Spellings;\n";
     OS << "  }\n";
     GenerateAppertainsTo(Attr, OS);
-    GenerateMutualExclusionsChecks(Attr, Records, OS, MergeOS);
+    GenerateMutualExclusionsChecks(Attr, Records, OS, MergeDeclOS, MergeStmtOS);
     GenerateLangOptRequirements(Attr, OS);
     GenerateTargetRequirements(Attr, Dupes, OS);
     GenerateSpellingIndexToSemanticSpelling(Attr, OS);
@@ -4008,16 +4026,27 @@ void EmitClangAttrParsedAttrImpl(RecordKeeper &Records, raw_ostream &OS) {
   // Generate the attribute match rules.
   emitAttributeMatchRules(PragmaAttributeSupport, OS);
 
-  OS << "#else // WANT_MERGE_LOGIC\n\n";
+  OS << "#elif defined(WANT_DECL_MERGE_LOGIC)\n\n";
 
   // Write out the declaration merging check logic.
   OS << "static bool DiagnoseMutualExclusions(Sema &S, const NamedDecl *D, "
      << "const Attr *A) {\n";
-  OS << MergeOS.str();
+  OS << MergeDeclOS.str();
   OS << "  return true;\n";
   OS << "}\n\n";
 
-  OS << "#endif // WANT_MERGE_LOGIC\n";
+  OS << "#elif defined(WANT_STMT_MERGE_LOGIC)\n\n";
+
+  // Write out the statement merging check logic.
+  OS << "static bool DiagnoseMutualExclusions(Sema &S, "
+     << "const SmallVectorImpl<const Attr *> &C) {\n";
+  OS << "  for (const Attr *A : C) {\n";
+  OS << MergeStmtOS.str();
+  OS << "  }\n";
+  OS << "  return true;\n";
+  OS << "}\n\n";
+
+  OS << "#endif\n";
 }
 
 // Emits the kind list of parsed attributes