[PATCH] D82982: [openmp] Move isAllowedClauseForDirective to tablegen + add clause version to OMP.td

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

Thanks for working on this.



================
Comment at: llvm/include/llvm/Frontend/Directive/DirectiveBase.td:64
 
+// Hold information about clause validity by version
+class VersionedClause<Clause c, int min = 1, int max = 0x7FFFFFFF> {
----------------
Please add punctuation to all comments.


================
Comment at: llvm/include/llvm/Frontend/Directive/DirectiveBase.td:65
+// Hold information about clause validity by version
+class VersionedClause<Clause c, int min = 1, int max = 0x7FFFFFFF> {
+  // Actual clause
----------------
Why not unsigned as in the original?


================
Comment at: llvm/include/llvm/Frontend/Directive/DirectiveBase.td:69
+
+  // Mininum version number where this clause is valid in the list.
+  int minVersion = min;
----------------
What does "the list" refer to?


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMP.td:278
+    VersionedClause<OMPC_NoWait>,
+    VersionedClause<OMPC_Allocate>];
 }
----------------
The closing `]` is inconsistently placed here and in some other cases.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:1881
-__OMP_DIRECTIVE_CLAUSE(flush, 50, ~0, release)
-// TODO This should ne `none` instead
-__OMP_DIRECTIVE_CLAUSE(flush, 1, ~0, flush)
----------------
This patch loses this TODO and the next one.  I'm not sure what they mean.  Do we need to keep them?


================
Comment at: llvm/test/TableGen/directive1.td:109
+// IMPL-NEXT:    assert(unsigned(C) <= llvm::tdl::Clause_enumSize);
+// IMPL-NEXT:    if (D == TDLD_dira && C == TDLC_clausea && 1 <= Version && 2147483647 >= Version)
+// IMPL-NEXT:      return true;
----------------
I know the original code used a giant if-else block, but shouldn't this be a switch?


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:56
     for (const auto &R : Records) {
       const auto Name = R->getValueAsString("name");
+      OS << "constexpr auto " << Prefix << getFormattedName(Name) << " = "
----------------
Any reason not to call `getFormattedName` here instead of twice below?


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:183
 
   const auto DefaultName = (*DefaultIt)->getValueAsString("name");
 
----------------
Call `getFormattedName(DefaultName)` once here?


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:214
+
+    OS << "  if (D == " << DirectivePrefix << getFormattedName(DirectiveName)
+       << " && C == " << ClausePrefix << getFormattedName(ClauseName) << " && "
----------------
Hoist `getFormattedName(DirectiveName)` out of the loop?


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:223
+// for the moment a copy of what was done in the OMPKinds.def. It can be
+// update in the future since we have more flexibility to generate code.
+void GenerateIsAllowedClause(const std::vector<Record *> &Directives,
----------------
I'd drop this comment's last two sentences, which don't seem like they will be meaningful after pushing.


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:296
+
+  OS << "\n"; // Empty line at end of file
 }
----------------
Why is an empty line needed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82982/new/

https://reviews.llvm.org/D82982