[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 4 inline comments as done.
jdoerfert added inline comments.


================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:23
 /// OpenMP directives.
-enum OpenMPDirectiveKind {
-#define OPENMP_DIRECTIVE(Name) \
-  OMPD_##Name,
-#define OPENMP_DIRECTIVE_EXT(Name, Str) \
-  OMPD_##Name,
-#include "clang/Basic/OpenMPKinds.def"
-  OMPD_unknown
-};
+using OpenMPDirectiveKind = llvm::omp::Directive;
 
----------------
fpetrogalli wrote:
> I am not a fan of `using` in header files in general contexts. It's mostly a stylistic preference. Why not use `llvm::omp::Directive` everywhere? It is not much longer than the substitution...
I did not do that to keep the diff as concise as possible. It is big enough as it is (I think) and this would add a lot of noise.
Do I need to change that now or are you OK with this? 


================
Comment at: clang/lib/Basic/OpenMPKinds.cpp:384
                                         OpenMPClauseKind CKind) {
-  assert(DKind <= OMPD_unknown);
+  assert(unsigned(DKind) <= unsigned(OMPD_unknown));
   assert(CKind <= OMPC_unknown);
----------------
fpetrogalli wrote:
> I really wish we would not have to add all these casts. They make the code ugly. I prefer `enum class` over `enum`, but if it results in this because most of the code uses enums, maybe is worth using just `enum`.
It seems so far most people are fine with the enum class if you now changed your mind and want to reopen this discussion, let me know please. 
FWIW, I'd argue we should minimize the use as numbers instead of going back on type safety. In the new code we only cast it a single time and there we actually want to use it as a number in a runtime call we generate.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:6625
 void OMPClauseWriter::VisitOMPClauseWithPreInit(OMPClauseWithPreInit *C) {
-  Record.push_back(C->getCaptureRegion());
+  Record.push_back(uint64_t(C->getCaptureRegion()));
   Record.AddStmt(C->getPreInitStmt());
----------------
fpetrogalli wrote:
> hum... is this really necessary? Why not make the type of the elements of `Record` the same as the type returned by `C->getCaptureregion()`?
Because this is generic clang serialization code. It has to lower to a byte sequence eventually, enum classes need explicit lowering.


================
Comment at: llvm/include/llvm/Frontend/OpenMPConstants.h:29-34
+/// Make the enum values available in the llvm::omp namespace. This allows us to
+/// write something like OMPD_parallel if we have a `using namespace omp`. At
+/// the same time we do not loose the strong type guarantees of the enum class,
+/// that is we cannot pass an unsigned as Directive without an explicit cast.
+#define OMP_DIRECTIVE(Enum, ...) constexpr auto Enum = omp::Directive::Enum;
+#include "llvm/Frontend/OpenMPKinds.def"
----------------
fpetrogalli wrote:
> Is this really necessary? What's wrong with writing `Directive::OMPD_parallel` when `using namespace omp`?
> 
> Also, isn't the information provided by `OMPD` a duplicate of `omp::Directive`? In my mind the acronym expands to **O**pen**MP** **D**irective...
> 
> Isn't the following more concise?
> 
> ```
> omp::Directive::parallel
> omp::Directive::declare_simd
> omp::Directive::whatever
> ```
> 
> IMHO, less is better :)
That doesn't work because some directives are C++ keywords.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853