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

[Valentin Clement via Phabricator via llvm-commits]

clementval added a comment.

Thanks for the review @jdenny. The patch is updated with your comments and some comments as well.



================
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
----------------
jdenny wrote:
> Why not unsigned as in the original?
TableGen type does not have an `unsigned` type so I went with the closest one.


================
Comment at: llvm/include/llvm/Frontend/Directive/DirectiveBase.td:69
+
+  // Mininum version number where this clause is valid in the list.
+  int minVersion = min;
----------------
jdenny wrote:
> What does "the list" refer to?
I updated the comment and dropped the list. I think it is clearer now.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMP.td:278
+    VersionedClause<OMPC_NoWait>,
+    VersionedClause<OMPC_Allocate>];
 }
----------------
jdenny wrote:
> The closing `]` is inconsistently placed here and in some other cases.
Good catch. I updated the file as well. 


================
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)
----------------
jdenny wrote:
> This patch loses this TODO and the next one.  I'm not sure what they mean.  Do we need to keep them?
I updated the patch to keep it. Same for `depobj`.


================
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;
----------------
jdenny wrote:
> I know the original code used a giant if-else block, but shouldn't this be a switch?
The idea was to migrate to TableGen with the exact same code generated and to update this in a follow up patch. It would be replace with at least a switch for the `Directive` and probably an inner switch for the `Clause` or we keep the `if`s inside the Directives' 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) << " = "
----------------
jdenny wrote:
> Any reason not to call `getFormattedName` here instead of twice below?
Updated in the patch.


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


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:296
+
+  OS << "\n"; // Empty line at end of file
 }
----------------
jdenny wrote:
> Why is an empty line needed?
Just to be consistent with clang-format in the generated file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82982