[PATCH] D83636: omp: Make OMP tablegen more like all other tablegens.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D83636#2148958 <https://reviews.llvm.org/D83636#2148958>, @clementval wrote:

> In D83636#2147351 <https://reviews.llvm.org/D83636#2147351>, @thakis wrote:
>
> > It puts the C++ code in C++ files :)
>
>
> The C++ code is currently in a C++ files (`OMP.cpp` that replaces `OMPConstants.cpp`). I don't get this comment.


We currently generate C++ code via tablegen, now we don't (generate as much C++ code but have the boilerplate manually written in OMP.cpp).

>> I agree there are merits to both approaches, but this is the one used ~everywhere else in LLVM and clang, so if it's a wash it seems good to stay consistent with the rest of the codebase.
> 
> MLIR is using similar technics for the dialects code. As I said, I don't have a strong opinion on this but I would like @jdoerfert to have look at this before going forward if you don't mind. It feels just weird to me to re-generate macros to be used to generate code that we can generate directly.

OK, I do not have much tablegen experience nor a strong opinion on this either way. To me, having macros for switches is not the problem since we generate the content via tablegen and not the macro cascade we used before. That makes the difference wrt. type safety and complex relationships. That said, it seems there are certain things we still generate as C++ and then include, e.g., the stuff guarded by `GET_IS_ALLOWED_CLAUSE_FOR_DIRECTIVE_IMPL`. So at the end we replace mostly switch code with generated macros, and use a manually written C++ boilerplate instead of generating it. Both seem sensible given the code size/complexity reduction. And then there is the fact that, if all things are equal, matching LLVM is for sure the preferred way.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMP.h:1
+#ifndef LLVM_OPENMP_OMP_H
+#define LLVM_OPENMP_OMP_H
----------------
file header missing.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMP.h:53
+} // namespace llvm
+#endif // LLVM_OpenMP_INC
----------------
Wrong guard naem?


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

https://reviews.llvm.org/D83636