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

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

jdenny marked 2 inline comments as done.
jdenny added inline 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
----------------
clementval wrote:
> jdenny wrote:
> > Why not unsigned as in the original?
> TableGen type does not have an `unsigned` type so I went with the closest one.
Ah, I thought I had seen it used elsewhere, but it looks like I'm mistaken.


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


================
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)
----------------
clementval wrote:
> 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`.
@jdoerfert : If I read git blame correctly, you wrote this TODO originally.  Can you help us to understand whether it's worth preserving?


================
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;
----------------
clementval wrote:
> 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. 
Sure, makes sense to do it in a separate patch.

It seems like both switches makes sense, but the inner switches might benefit from default cases.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82982