[PATCH] D81736: [openmp] Base of tablegen generated OpenMP common declaration

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added reviewers: fghanim, ABataev, jdenny, hfinkel, jhuber6, kiranchandramohan, kiranktp.
jdoerfert added a comment.

This is really cool :)

In D81736#2090419 <https://reviews.llvm.org/D81736#2090419>, @clementval wrote:

> @jdoerfert Some tests in clang relies on the position of the specific enum in the declaration. TableGen is sorting the records so the enum is generated in alphabetical order therefore the enum position is changed. Is it fine to update the test with smith like `{{.*}}` instead of the specific id?


Yes, that's fine.

I left some comments and added some more reviewers.



================
Comment at: llvm/include/llvm/Frontend/Directive/DirectiveBase.td:69
+  list<Clause> requiredClauses = ?;
+}
----------------
I really like we have this abstraction now and also a nice way to encode some constraints, e.g., required clauses, right away :)


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMP.td:496
+def OMP_EndDeclareVariant : Directive<"end declare variant"> {}
+def OMP_Unknown : Directive<"unknown"> {}
----------------
I guess we go over this in a follow up and use the `requiredClauses` and similar features, that seems reasonable to me. 


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:50
-#include "llvm/Frontend/OpenMP/OMPKinds.def"
-
 /// IDs for all omp runtime library (RTL) functions.
----------------
While the MACRO MAGIC worked surprisingly well, I'm glad it is going away.


================
Comment at: llvm/test/TableGen/directive.td:34
+// CHECK-NEXT: }
+// CHECK-NEXT: }
----------------
How does the `allowedClauses` affect the result?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81736