[PATCH] D82405: [openmp] Move Directive and Clause helper function to tablegen

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

jdenny added a comment.

In D82405#2112522 <https://reviews.llvm.org/D82405#2112522>, @clementval wrote:

> In D82405#2112482 <https://reviews.llvm.org/D82405#2112482>, @jdoerfert wrote:
>
> > In D82405#2112451 <https://reviews.llvm.org/D82405#2112451>, @clementval wrote:
> >
> > > In D82405#2112272 <https://reviews.llvm.org/D82405#2112272>, @jdoerfert wrote:
> > >
> > > > So the idea is we generate a single .inc file with multiple `ifdef` regions to choose from? Is that what is usually done? I would have thought multiple files are nicer but no strong feeling.
> > >
> > >
> > > I have no strong opinion on this as well. MLIR is generating two files for their dialects (1 ``.h.inc` and `.cpp.inc) but use the sections as well. We will probably need the sections as well even if we do two or more files.
> >
> >
> > Why do we need sections if we generate multiple files?
>
>
> So far if we have a .h.inc and a .cpp.inc there is no need for sections for the common parts related to enumerations but I can imagine that some part in clang or Flang which currently use OMPKinds.def need replacement code that are quite specific and might reside in a section. Does it make sense?


That would be for tweaking parts of the content not for selecting entirely different content, right?

It seems unintuitive to me to force two completely different files into one file in this manner, but I have no strong opinion either.  If that's the norm, then fine by me.



================
Comment at: llvm/include/llvm/Frontend/Directive/DirectiveBase.td:49
 
+  // Alternative name
+  string alternativeName = "";
----------------
Could this comment clarify briefly how the alternative name might be used?  Likewise for the one in `Directive`.

Also, minor nit: the comments in this file are not consistent about punctuation.  My read of the style standard is that [[ http://llvm.org/docs/CodingStandards.html#commenting | punctuation is preferred ]].


================
Comment at: llvm/include/llvm/Frontend/Directive/DirectiveBase.td:55
 
   // Is clause implicit?
   bit isImplicit = 0;
----------------
The effect of `isImplicit` and `isDefault` on `get*ClauseKind` is subtle.  Can this be documented here?


================
Comment at: llvm/test/TableGen/directive1.td:28
+
+// CHECK: #ifndef LLVM_Tdl_INC
+// CHECK-NEXT: #define LLVM_Tdl_INC
----------------
Is this CHECK instead of CHECK-NEXT because there are other lines before that shouldn't be checked?  If, instead, this just handles empty lines, CHECK-EMPTY can do that but won't let additional non-empty lines slip in later.

Likewise for other cases below.


================
Comment at: llvm/test/TableGen/directive1.td:64
+// CHECK-NEXT: Directive getTdlDirectiveKind(llvm::StringRef Str) {
+// CHECK-NEXT: return llvm::StringSwitch<Directive>(Str)
+// CHECK-NEXT: .Case("dira",TDLD_dira)
----------------
Please consider using indentation within FileCheck directives like this one so that the embedded C++ here is more readable.

Also maybe consider using `FileCheck -strict-whitespace -match-full-lines` to be sure unintentional text or formatting doesn't slip in later.  I'll admit, most tests don't do this so that they're less brittle, but I feel like that's not likely an issue here.

Of course, some of that might need to go in a later review so this review remains easy to read.


================
Comment at: llvm/test/TableGen/directive1.td:88
+// CHECK-NEXT: }
+// CHECK-NEXT: llvm_unreachable("Invalid Tdl directive kind");
+// CHECK-NEXT: }
----------------
directive -> clause


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:248
+  GenerateGetKind(Directives, OS, "Directive", DirectivePrefix, LanguageName,
+                  false);
+
----------------
[[ http://llvm.org/docs/CodingStandards.html#comment-formatting | Please use the style ]] `/*ParamName=*/constant` for arguments like `false`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82405