The new OpenMPConstants.h is a location for all OpenMP related constants
(and helpers) to live.
This patch moves the directives there (the enum OpenMPDirectiveKind) and
rewires Clang to use the new location.
Initially part of D69785.
The new OpenMPConstants.h is a location for all OpenMP related constants
(and helpers) to live.
This patch moves the directives there (the enum OpenMPDirectiveKind) and
rewires Clang to use the new location.
Initially part of D69785.
Buildable 40784 | |
Build 40916: arc lint + arc unit |
clang/lib/Basic/OpenMPKinds.cpp | ||
---|---|---|
408 | Why assert is removed? | |
clang/lib/Parse/ParseOpenMP.cpp | ||
51–60 | Why do we need this? | |
llvm/include/llvm/IR/OpenMPConstants.h | ||
40 ↗ | (On Diff #227890) |
|
clang/lib/Basic/OpenMPKinds.cpp | ||
---|---|---|
408 | I can add it back, I mean it would look like this now: `assert(unsigned(DKind) <= unsigned(OMPD_unknown));` (I thought that the idea of enum classes is not to need it but we can keep it). | |
clang/lib/Parse/ParseOpenMP.cpp | ||
51–60 | First, I'm open for suggestions on how to do this in a nicer way :) Why I did it: | |
llvm/include/llvm/IR/OpenMPConstants.h | ||
40 ↗ | (On Diff #227890) |
|
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4017–4018 | 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. | |
llvm/lib/IR/OpenMPIRBuilder.cpp | ||
11 ↗ | (On Diff #227890) | Implemented in OpenMPConstants.cpp instead? Functions look usable independent of MPIRBuilder. |
Again, better to split into 2 patches, one for LLVM and one for clang.
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4017–4018 | Why map? It can be represented as a simple c-style array without any problems. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4017–4018 | No, these are not enums that auto-convert to unsigned. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4017–4018 | Convert them manually. Replacing the simple hash array with map does not look like a good solution. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4017–4018 | 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.". |
I split the other patch, but this one cannot be split in a reasonable (=testable) manner. Splitting a patch only to have smaller diffs is not the goal.
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4017–4018 | You already introduced special wrapper class in the same file, maybe you can reuse for implicit conversions? |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4017–4018 | We are introducing an EnumeratedArray class in https://reviews.llvm.org/D68827#change-XphX8PAWFr8V . It should reduce the need for casting and OpenMPDirectiveKindExWrapper. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4017–4018 | 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. |
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.
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4017–4018 | Then better to use llvm::IndexedMap or llvm::DenseMap |
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.
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4017–4018 | It will not matter but sure. |
Testing is not a problem for NFC LLVM part of the patch, I think. Plus, this whole patch is NFC.
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4009 | ArgumentType |
That is not a reason not to test it. And again, there are arguments for a combined patch even beyond testing.
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4009 | it has to be argument_type. |
LG with a nit
llvm/include/llvm/IR/OpenMPConstants.h | ||
---|---|---|
1 ↗ | (On Diff #228275) | Just OpenMPConstants.h? |
Create llvm/libFrontend, move code there, make shared library builds work.
llvm/IR/ is an unfortunate location as we cannot use llvm utility functions
without introducing dependences between the libraries that are unwanted.
The new libFrontend should not only house the OpenMP-IR-Builder but other code
parts shared between different frontends later on.
clang/include/clang/Basic/OpenMPKinds.h | ||
---|---|---|
23 | 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... | |
clang/lib/Basic/OpenMPKinds.cpp | ||
384 | 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. | |
clang/lib/Serialization/ASTWriter.cpp | ||
6625 | hum... is this really necessary? Why not make the type of the elements of Record the same as the type returned by C->getCaptureregion()? | |
llvm/include/llvm/Frontend/OpenMPConstants.h | ||
29–34 | 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 OpenMP Directive... Isn't the following more concise? omp::Directive::parallel omp::Directive::declare_simd omp::Directive::whatever IMHO, less is better :) |
clang/include/clang/Basic/OpenMPKinds.h | ||
---|---|---|
23 | 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. | |
clang/lib/Basic/OpenMPKinds.cpp | ||
384 | 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. | |
clang/lib/Serialization/ASTWriter.cpp | ||
6625 | Because this is generic clang serialization code. It has to lower to a byte sequence eventually, enum classes need explicit lowering. | |
llvm/include/llvm/Frontend/OpenMPConstants.h | ||
29–34 | That doesn't work because some directives are C++ keywords. |