[OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[jdoerfert]

Summary

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.

[ABataev]

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)

1. Better to return StringRef, I think.
2. Do we really need these 2 convert functions here? Are we going to use them in LLVM or just in clang?

[jdoerfert]

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:
The code below uses two enums as if they were unsigned numbers and one of them is now an enum class
which requires you to do explicit casts. Without this trickery here we would need to sprinkle unsigned(XXXX) in the code below, especially the array F which I did not want to do.

llvm/include/llvm/IR/OpenMPConstants.h

40 ↗

(On Diff #227890)

1. Can do, I just moved them in this patch.
2. We will reuse them in Flang so they should be moved (and it seems natural to keep these where the enums are defined.)

[JonChesterfield]

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.

[jdoerfert]

Addressed review comments, simplified some changes, moved file, ...

[ABataev]

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.

[jdoerfert]

clang/lib/Sema/SemaOpenMP.cpp

4017–4018

No, these are not enums that auto-convert to unsigned.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

4017–4018

Convert them manually. Replacing the simple hash array with map does not look like a good solution.

[jdoerfert]

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

[jdoerfert]

In D69853#1734620, @ABataev wrote:

Again, better to split into 2 patches, one for LLVM and one for clang.

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.

[ABataev]

In D69853#1734760, @jdoerfert wrote:

In D69853#1734620, @ABataev wrote:

Again, better to split into 2 patches, one for LLVM and one for clang.

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.

We don't need to test it. Just split it into 2 parts, llvm and clang, nothing else.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

4017–4018

You already introduced special wrapper class in the same file, maybe you can reuse for implicit conversions?

[Meinersbur]

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.

[jdoerfert]

More updates

[jdoerfert]

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.

[jdoerfert]

working version

[jdoerfert]

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

[ABataev]

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

clang/lib/Sema/SemaOpenMP.cpp

4017–4018

Then better to use llvm::IndexedMap or llvm::DenseMap

[jdoerfert]

In D69853#1737264, @ABataev wrote:

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

clang/lib/Sema/SemaOpenMP.cpp

4017–4018

It will not matter but sure.

[jdoerfert]

Use an llvm::IndexedMap instead of std::map

[ABataev]

In D69853#1737319, @jdoerfert wrote:

In D69853#1737264, @ABataev wrote:

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

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

[jdoerfert]

In D69853#1737426, @ABataev wrote:

In D69853#1737319, @jdoerfert wrote:

In D69853#1737264, @ABataev wrote:

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

Testing is not a problem for NFC LLVM part of the patch, I think. Plus, this whole patch is NFC.

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.

[ABataev]

LG with a nit

llvm/include/llvm/IR/OpenMPConstants.h

1 ↗

(On Diff #228275)

Just OpenMPConstants.h?

[jdoerfert]

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.

[fpetrogalli]

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 :)

[jdoerfert]

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.
Do I need to change that now or are you OK with this?

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

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.