[PATCH] D140292: [OpenMP] Migrate OpenMPOffloadMappingFlags from Clang CodeGen to OMPConstants

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked 2 inline comments as done.
TIFitis added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:193
-      OMP_TGT_EXEC_MODE_GENERIC | OMP_TGT_EXEC_MODE_SPMD,
-  LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue */ OMP_TGT_EXEC_MODE_GENERIC_SPMD)
 };
----------------
jdoerfert wrote:
> TIFitis wrote:
> > jdoerfert wrote:
> > > TIFitis wrote:
> > > > I am not sure if this change is safe. It can be avoided by making `OpenMPOffloadMappingFlags` an enum class.
> > > Why do you need to change this enum at all?
> > Otherwise you'd have two declarations of LLVM_MARK_AS_BITMASK_ENUM in the same namespace which is an error ofc. 
> > 
> > This is because they are both enums which spill the declarations to the enclosing namespace, I.e. llvm::omp
> But we are using that type in binary operations w/o cast. Does this change then not break existing code?
Going by the comments `LLVM_MARK_AS_BITMASK_ENUM` should be used to mark the largest individual enum, don't see any restrictions on it being used with multiple enum's in the same namespace.

`check-all` is also clean. I don't see any evidence of this being unsafe.

Changing `OpenMPOffloadMappingFlags` to enum class however, would be the safest thing to do but that would introduce a lot of ugly static_cast everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140292