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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D69853#1737218 <https://reviews.llvm.org/D69853#1737218>, @jdoerfert wrote:

> Are there  blocking issues on this one or can we go ahead and adjust minor details as we go?


I still think it would be good to separate patches into the LLVM part and into the clang part and commit them separately? E.g. flang people may try to look for the commits for constant in LLVM, but not for the clang changes. It will be much easier for them to skip the changes in clang.



================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011
   unsigned NamedModifiersNumber = 0;
-  SmallVector<const OMPIfClause *, OMPC_unknown + 1> FoundNameModifiers(
-      OMPD_unknown + 1);
+  SmallVector<const OMPIfClause *, unsigned(OMPC_unknown) + 1>
+  FoundNameModifiers(unsigned(OMPD_unknown) + 1);
----------------
jdoerfert wrote:
> Meinersbur wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > JonChesterfield wrote:
> > > > > > > > I wonder if it would be worth wrapping the accesses to FoundNameModifiers in functor that does the enum class to unsigned conversion. E.g. a class instance that contains the small vector and exposes operator[] that takes the enum class.
> > > > > > > > 
> > > > > > > > FoundNameModifiers[unsigned(val)] is quite a lot of line noise.
> > > > > > > Why `map`? It can be represented as a simple c-style array without any problems.
> > > > > > No, these are not enums that auto-convert to unsigned.
> > > > > Convert them manually. Replacing the simple hash array with map does not look like a good solution.
> > > > I feel this this is a micro optimization that will make the code less maintainable and, as Jon noted, "FoundNameModifiers[unsigned(val)] is quite a lot of line noise.". 
> > > > Take a look at the first version and let me know if you want to go back to that one for this part, if so, I can do that.
> > > You already introduced special wrapper class in the same file, maybe you can reuse for implicit conversions?
> > We are introducing an `EnumeratedArray` class in https://reviews.llvm.org/D68827#change-XphX8PAWFr8V . It should reduce the need for casting and `OpenMPDirectiveKindExWrapper`.
> Arguably, the easiest and cleanest way is to use a `map` to *map directives to if clauses*.
> 
> The wrapper is in a different file but I can write another one for here.
> 
> EnumeratedArray could probably be used here, it is not in yet though.
Then better to use `llvm::IndexedMap` or `llvm::DenseMap`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853