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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.

In D69853#1737264 <https://reviews.llvm.org/D69853#1737264>, @ABataev wrote:

> 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.


But they might also want to see how to interact with the constants so having the clang parts in there is good. I do not see a clear benefit, e.g., Flang ppl might or might not prefer a separate patch, but I see a clear drawback (testing wise) so I still don't believe splitting the right thing.



================
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);
----------------
ABataev wrote:
> 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`
It will not matter but sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853