[openmp][openacc] Check for duplicate clauses for directive

[clementval]

Summary

Check for duplicate clauses associated with directive. Clauses can appear only once
in the 4 lists associated with each directive (allowedClauses, allowedOnceClauses,
allowedExclusiveClauses, requiredClauses). Duplicates were already present (removed with this
patch) or were introduce in new patches by mistake (D89861).

[kiranchandramohan]

This is a good verification check. A few questions.

llvm/include/llvm/TableGen/DirectiveEmitter.h

106

Any issues with moving this to the base class?

llvm/test/TableGen/directive3.td

12–13

Nit: Extra line/lines?

llvm/utils/TableGen/DirectiveEmitter.cpp

119

Wouldn't this lead to an O(N^2) loop. Can we do better?

[clementval]

llvm/include/llvm/TableGen/DirectiveEmitter.h

106

I'll move it.

llvm/utils/TableGen/DirectiveEmitter.cpp

119

CrtClauses is a StringSet implemented on the top of StringMap. The lookup should be constant (O(1)) so inside the loop we should be O(N).

[clementval]

Address review comment

[kiranchandramohan]

llvm/test/TableGen/directive3.td

31

Nit: Is this second error giving any additional information? Is this twin errors so that we get all occurrences of errors rather than fail at the first one?

llvm/utils/TableGen/DirectiveEmitter.cpp

119

Ahh, OK. Completely missed that.
Nit: Would using insert and checking the return value be better than using find and insert?

150–174

Is this code from 151 to 162 similar for all occurrences of HasDuplicateClausesInDirectives? If so would it be better to pull it into a function and use that function?

[clementval]

Address review comments

llvm/test/TableGen/directive3.td

31

The first error is the specific error triggered for each duplicate found. The last one is the general error mentioning that one or more duplicates have been found. I'm happy to remove the global error if it is confusing.

llvm/utils/TableGen/DirectiveEmitter.cpp

150–174

I extract it to a function. I plan to add a wrapper for Records so we can get rid of all the Records.getAllDerivedDefinitions("DirectiveLanguage"); and have simple accessor and also pass just a reference to the records to all those functions that now takes vector of Records. But that will be in a next patch.

[kiranchandramohan]

LGTM.

llvm/utils/TableGen/DirectiveEmitter.cpp

150–174

OK

[Paul-C-Anagnostopoulos]

When I run the TableGen tests on master, I get the output below. I just rebased master to be sure I am up to date.

 -- Testing: 1 tests, 1 workers --
FAIL: LLVM :: TableGen/directive3.td (1 of 1)
******************** TEST 'LLVM :: TableGen/directive3.td' FAILED ********************
Script:
--
: 'RUN: at line 1';   not d:\llvm\build\bin\llvm-tblgen.exe -gen-directive-decl -I C:\LLVM\llvm-project\llvm\test\TableGen/../../include C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td 2>&1 | d:\llvm\build\bin\filecheck.exe -match-full-lines C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td
: 'RUN: at line 2';   not d:\llvm\build\bin\llvm-tblgen.exe -gen-directive-impl -I C:\LLVM\llvm-project\llvm\test\TableGen/../../include C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td 2>&1 | d:\llvm\build\bin\filecheck.exe -match-full-lines C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td
: 'RUN: at line 3';   not d:\llvm\build\bin\llvm-tblgen.exe -gen-directive-gen -I C:\LLVM\llvm-project\llvm\test\TableGen/../../include C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td 2>&1 | d:\llvm\build\bin\filecheck.exe -match-full-lines C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "not" "d:\llvm\build\bin\llvm-tblgen.exe" "-gen-directive-decl" "-I" "C:\LLVM\llvm-project\llvm\test\TableGen/../../include" "C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td"
note: command had no output on stdout or stderr
error: command failed with exit status: 1
$ "d:\llvm\build\bin\filecheck.exe" "-match-full-lines" "C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td"
# command stderr:
C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td:29:11: error: CHECK: expected string not found in input
// CHECK: error: Clause TDLC_ClauseA already defined on directive TDL_DirA
          ^
<stdin>:1:1: note: scanning from here
#ifndef LLVM_TdlError_INC
^

Input file: <stdin>
Check file: C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td

-dump-input=help explains the following input dump.

Input was:
<<<<<<
          1: #ifndef LLVM_TdlError_INC
check:29     X~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
          2: #define LLVM_TdlError_INC
check:29     ~~~~~~~~~~~~~~~~~~~~~~~~~
          3: 
check:29     ~
          4: namespace llvm {
check:29     ~~~~~~~~~~~~~~~~
          5: class StringRef;
check:29     ~~~~~~~~~~~~~~~~
          6: namespace TdlError {
check:29     ~~~~~~~~~~~~~~~~~~~~
          .
          .
          .
>>>>>>

error: command failed with exit status: 1

--

********************
********************
Failed Tests (1):
  LLVM :: TableGen/directive3.td


Testing Time: 0.31s
  Failed: 1

[clementval]

In D90241#2362521, @Paul-C-Anagnostopoulos wrote:

When I run the TableGen tests on master, I get the output below. I just rebased master to be sure I am up to date.

This is weird because windows buildbots are not triggering the error you are seeing. Did you try with a clean build? Your lit log looks like that error was not triggered and the backend generated the file anyway.

[Paul-C-Anagnostopoulos]

How strange. I checked out master, rebased it, and ran the test. It worked like a charm. I have no explanation.

Sorry for the hassle.

llvm/include/llvm/Frontend/OpenACC/ACC.td

let allowedClauses = [
     VersionedClause<ACCC_Attach>
   ];
   let allowedOnceClauses = [
     VersionedClause<ACCC_Async>,
     VersionedClause<ACCC_Collapse>,
     VersionedClause<ACCC_Default>,
     VersionedClause<ACCC_Gang>,
     VersionedClause<ACCC_If>,
-    VersionedClause<ACCC_Independent>,
     VersionedClause<ACCC_NumGangs>,
     VersionedClause<ACCC_NumWorkers>,
     VersionedClause<ACCC_Reduction>,
     VersionedClause<ACCC_Self>,
     VersionedClause<ACCC_Tile>,
     VersionedClause<ACCC_Vector>,
     VersionedClause<ACCC_VectorLength>,
     VersionedClause<ACCC_Wait>,

llvm/include/llvm/TableGen/DirectiveEmitter.h

std::string getFormattedName() {
     StringRef Name = Def->getValueAsString("name");
     std::string N = Name.str();
     std::replace(N.begin(), N.end(), ' ', '_');
     return N;
   }
 
   bool isDefault() const { return Def->getValueAsBit("isDefault"); }
 
+  // Returns the record name.
+  const StringRef getRecordName() const { return Def->getName(); }
+
 protected:
   const llvm::Record *Def;
 };
 
 // Wrapper class that contains a Directive's information defined in
 // DirectiveBase.td and provides helper methods for accessing it.
 class Directive : public BaseRecord {
 public:
std::vector<Record *> getAllowedExclusiveClauses() const {
     return Def->getValueAsListOfDefs("allowedExclusiveClauses");
   }
 
   std::vector<Record *> getRequiredClauses() const {
     return Def->getValueAsListOfDefs("requiredClauses");
   }
 };
 
 // Wrapper class that contains Clause's information defined in DirectiveBase.td
 // and provides helper methods for accessing it.
 class Clause : public BaseRecord {
 public:
   explicit Clause(const llvm::Record *Def) : BaseRecord(Def) {}
 
   // Optional field.
   StringRef getClangClass() const {
     return Def->getValueAsString("clangClass");

llvm/test/TableGen/directive3.td

+// RUN: not llvm-tblgen -gen-directive-decl -I %p/../../include %s 2>&1 | FileCheck -match-full-lines %s
+// RUN: not llvm-tblgen -gen-directive-impl -I %p/../../include %s 2>&1 | FileCheck -match-full-lines %s
+// RUN: not llvm-tblgen -gen-directive-gen -I %p/../../include %s 2>&1 | FileCheck -match-full-lines %s
+
+include "llvm/Frontend/Directive/DirectiveBase.td"
+
+def TestDirectiveLanguage : DirectiveLanguage {
+  let name = "TdlError";
+}
+
+def TDLC_ClauseA : Clause<"clausea"> {
+  let isDefault = 1;
+}
+
+def TDLC_ClauseB : Clause<"clauseb"> {
+}
+
+def TDL_DirA : Directive<"dira"> {
+  let allowedClauses = [
+    VersionedClause<TDLC_ClauseA>,
+    VersionedClause<TDLC_ClauseB>
+  ];
+  let allowedOnceClauses = [
+    VersionedClause<TDLC_ClauseA>
+  ];
+  let isDefault = 1;
+}
+
+// CHECK: error: Clause TDLC_ClauseA already defined on directive TDL_DirA
+// CHECK: error: One or more clauses are defined multiple times on directive TDL_DirA

llvm/utils/TableGen/DirectiveEmitter.cpp

if (DirLang.hasMakeEnumAvailableInNamespace()) {
       }
       EnumHelperFuncs += (llvm::Twine(EnumName) + llvm::Twine(" get") +
                           llvm::Twine(EnumName) + llvm::Twine("(StringRef);\n"))
                              .str();
     }
   }
 }
 
-// Generate the declaration section for the enumeration in the directive
-// language
-void EmitDirectivesDecl(RecordKeeper &Records, raw_ostream &OS) {
+bool HasDuplicateClauses(const std::vector<Record *> &Clauses,
+                         const Directive &Directive,
+                         llvm::StringSet<> &CrtClauses) {
+  bool hasError = false;
+  for (const auto &C : Clauses) {
+    VersionedClause VerClause{C};
+    const auto insRes = CrtClauses.insert(VerClause.getClause().getName());
+    if (!insRes.second) {
+      PrintError("Clause " + VerClause.getClause().getRecordName() +
+                 " already defined on directive " + Directive.getRecordName());
+      hasError = true;
+    }
+  }
+  return hasError;
+}
 
+bool HasDuplicateClausesInDirectives(const std::vector<Record *> &Directives) {
+  for (const auto &D : Directives) {
+    Directive Dir{D};
+    llvm::StringSet<> Clauses;
+    if (HasDuplicateClauses(Dir.getAllowedClauses(), Dir, Clauses) ||
+        HasDuplicateClauses(Dir.getAllowedOnceClauses(), Dir, Clauses) ||
+        HasDuplicateClauses(Dir.getAllowedExclusiveClauses(), Dir, Clauses) ||
+        HasDuplicateClauses(Dir.getRequiredClauses(), Dir, Clauses)) {
+      PrintFatalError(
+          "One or more clauses are defined multiple times on directive " +
+          Dir.getRecordName());
+      return true;
+    }
+  }
+  return false;
+}
+
+// Check consitency of records. Return true if an error has been detected.
+// Return false if the records are valid.
+bool CheckRecordsValidity(RecordKeeper &Records) {
   const auto &DirectiveLanguages =
       Records.getAllDerivedDefinitions("DirectiveLanguage");
 
   if (DirectiveLanguages.size() != 1) {
     PrintError("A single definition of DirectiveLanguage is needed.");
-    return;
+    return true;
   }
 
+  const auto &Directives = Records.getAllDerivedDefinitions("Directive");
+  return HasDuplicateClausesInDirectives(Directives);
+}
+
+// Generate the declaration section for the enumeration in the directive
+// language
+void EmitDirectivesDecl(RecordKeeper &Records, raw_ostream &OS) {
+
+  if (CheckRecordsValidity(Records))
+    return;
+
+  const auto &DirectiveLanguages =
+      Records.getAllDerivedDefinitions("DirectiveLanguage");
+
   DirectiveLanguage DirLang{DirectiveLanguages[0]};
+  const auto &Directives = Records.getAllDerivedDefinitions("Directive");
 
   OS << "#ifndef LLVM_" << DirLang.getName() << "_INC\n";
   OS << "#define LLVM_" << DirLang.getName() << "_INC\n";
 
   if (DirLang.hasEnableBitmaskEnumInNamespace())
     OS << "\n#include \"llvm/ADT/BitmaskEnum.h\"\n";
 
   OS << "\n";
   OS << "namespace llvm {\n";
   OS << "class StringRef;\n";
 
   // Open namespaces defined in the directive language
   llvm::SmallVector<StringRef, 2> Namespaces;
   llvm::SplitString(DirLang.getCppNamespace(), Namespaces, "::");
   for (auto Ns : Namespaces)
     OS << "namespace " << Ns << " {\n";
 
   if (DirLang.hasEnableBitmaskEnumInNamespace())
     OS << "\nLLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();\n";
 
   // Emit Directive enumeration
-  const auto &Directives = Records.getAllDerivedDefinitions("Directive");
   GenerateEnumClass(Directives, OS, "Directive", DirLang.getDirectivePrefix(),
                     DirLang);
 
   // Emit Clause enumeration
   const auto &Clauses = Records.getAllDerivedDefinitions("Clause");
   GenerateEnumClass(Clauses, OS, "Clause", DirLang.getClausePrefix(), DirLang);
 
   // Emit ClauseVal enumeration
void EmitDirectivesFlangImpl(const std::vector<Record *> &Directives,
   GenerateFlangClauseDump(Clauses, DirectiveLanguage, OS);
 
   GenerateFlangClauseUnparse(Clauses, DirectiveLanguage, OS);
 }
 
 // Generate the implemenation section for the enumeration in the directive
 // language.
 void EmitDirectivesGen(RecordKeeper &Records, raw_ostream &OS) {
+  if (CheckRecordsValidity(Records))
+    return;
 
   const auto &DirectiveLanguages =
       Records.getAllDerivedDefinitions("DirectiveLanguage");
-
-  if (DirectiveLanguages.size() != 1) {
-    PrintError("A single definition of DirectiveLanguage is needed.");
-    return;
-  }
-
   const auto &Directives = Records.getAllDerivedDefinitions("Directive");
   const auto &Clauses = Records.getAllDerivedDefinitions("Clause");
   DirectiveLanguage DirectiveLanguage{DirectiveLanguages[0]};
   EmitDirectivesFlangImpl(Directives, Clauses, OS, DirectiveLanguage);
 }
 
 // Generate the implemenation for the enumeration in the directive
 // language. This code can be included in library.
 void EmitDirectivesImpl(RecordKeeper &Records, raw_ostream &OS) {
+  if (CheckRecordsValidity(Records))
+    return;
 
   const auto &DirectiveLanguages =
       Records.getAllDerivedDefinitions("DirectiveLanguage");
-
-  if (DirectiveLanguages.size() != 1) {
-    PrintError("A single definition of DirectiveLanguage is needed.");
-    return;
-  }
-
   const auto &Directives = Records.getAllDerivedDefinitions("Directive");
-
   DirectiveLanguage DirLang = DirectiveLanguage{DirectiveLanguages[0]};
 
   const auto &Clauses = Records.getAllDerivedDefinitions("Clause");
 
   if (!DirLang.getIncludeHeader().empty())
     OS << "#include \"" << DirLang.getIncludeHeader() << "\"\n\n";
 
   OS << "#include \"llvm/ADT/StringRef.h\"\n";