From 85c78a5df44f84fecb7e5d7d833b12541ac579a8 Mon Sep 17 00:00:00 2001 From: Aaron Ballman Date: Fri, 9 Apr 2021 07:35:15 -0400 Subject: [PATCH 1/3] Correct the tablegen for checking mutually exclusive stmt attrs 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. --- clang/include/clang/Basic/Attr.td | 8 ++ clang/include/clang/Sema/ParsedAttr.h | 13 ++- clang/lib/Sema/ParsedAttr.cpp | 4 - clang/lib/Sema/SemaDecl.cpp | 4 +- clang/lib/Sema/SemaStmtAttr.cpp | 55 +++-------- clang/test/SemaSYCL/intel-fpga-loops.cpp | 15 ++- clang/utils/TableGen/ClangAttrEmitter.cpp | 109 ++++++++++++++-------- 7 files changed, 108 insertions(+), 100 deletions(-) diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td index 74fb6e7045fd..4a4f14e61733 100644 --- a/clang/include/clang/Basic/Attr.td +++ b/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">]; diff --git a/clang/include/clang/Sema/ParsedAttr.h b/clang/include/clang/Sema/ParsedAttr.h index a90131d47849..3307d1db19fc 100644 --- a/clang/include/clang/Sema/ParsedAttr.h +++ b/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> diff --git a/clang/lib/Sema/ParsedAttr.cpp b/clang/lib/Sema/ParsedAttr.cpp index 0bf725269517..845af9a2792a 100644 --- a/clang/lib/Sema/ParsedAttr.cpp +++ b/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); diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 71ceb8aefefc..433edf626166 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/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, diff --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp index 15a86309677d..32c6edc77168 100644 --- a/clang/lib/Sema/SemaStmtAttr.cpp +++ b/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 &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 -static void CheckMutualExclusionSYCLLoopAttribute( - Sema &S, const SmallVectorImpl &Attrs) { - std::pair 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(I)) - FirstSeen = I; - - // Remember if we've seen either of the attribute types. - if (isa(I)) - SeenAttrs.first = true; - else if (isa(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 &Attrs) { CheckForDuplicationSYCLLoopAttribute( @@ -573,21 +559,6 @@ static void CheckForIncompatibleSYCLLoopAttributes( CheckForDuplicationSYCLLoopAttribute( S, Attrs); CheckForDuplicationSYCLLoopAttribute(S, Attrs, false); - CheckMutualExclusionSYCLLoopAttribute( - S, Attrs); - CheckMutualExclusionSYCLLoopAttribute( - S, Attrs); - CheckMutualExclusionSYCLLoopAttribute( - S, Attrs); - CheckMutualExclusionSYCLLoopAttribute(S, Attrs); - CheckMutualExclusionSYCLLoopAttribute(S, - Attrs); - CheckRedundantSYCLIntelFPGAIVDepAttrs(S, Attrs); CheckForDuplicationSYCLLoopAttribute(S, Attrs); } diff --git a/clang/test/SemaSYCL/intel-fpga-loops.cpp b/clang/test/SemaSYCL/intel-fpga-loops.cpp index 082899e84ab2..6bec33ff00b7 100644 --- a/clang/test/SemaSYCL/intel-fpga-loops.cpp +++ b/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; diff --git a/clang/utils/TableGen/ClangAttrEmitter.cpp b/clang/utils/TableGen/ClangAttrEmitter.cpp index 30f1ea103c90..f0e74da4e6fb 100644 --- a/clang/utils/TableGen/ClangAttrEmitter.cpp +++ b/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 &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(St)) {\n"; - OS << " const ArrayRef &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 &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 From 9ecdad22a327476d3cef8f95d80e7be3b659e2af Mon Sep 17 00:00:00 2001 From: Aaron Ballman Date: Fri, 9 Apr 2021 08:21:30 -0400 Subject: [PATCH 2/3] Adding missing test coverage for [[likely, unlikely]] --- clang/test/SemaCXX/attr-likelihood.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/clang/test/SemaCXX/attr-likelihood.cpp b/clang/test/SemaCXX/attr-likelihood.cpp index d2f95d17069c..03ba301251d7 100644 --- a/clang/test/SemaCXX/attr-likelihood.cpp +++ b/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 From d3d99c60b7a204ae6e24010cc3b4b53e0846a8ff Mon Sep 17 00:00:00 2001 From: Aaron Ballman Date: Fri, 9 Apr 2021 11:14:15 -0400 Subject: [PATCH 3/3] Make the code uglier to appease clang-format --- clang/utils/TableGen/ClangAttrEmitter.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/utils/TableGen/ClangAttrEmitter.cpp b/clang/utils/TableGen/ClangAttrEmitter.cpp index f0e74da4e6fb..29d358ebd12c 100644 --- a/clang/utils/TableGen/ClangAttrEmitter.cpp +++ b/clang/utils/TableGen/ClangAttrEmitter.cpp @@ -3758,9 +3758,9 @@ static void GenerateMutualExclusionsChecks(const Record &Attr, << (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 << ", "; }); + interleave( + StmtAttrs, [&](const std::string &Name) { MergeStmtOS << Name; }, + [&] { MergeStmtOS << ", "; }); MergeStmtOS << ">(Check); });\n"; MergeStmtOS << " if (Iter != C.end()) {\n"; MergeStmtOS << " S.Diag((*Iter)->getLocation(), "