Page MenuHomePhabricator

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

Authored by jdoerfert on Nov 5 2019, 8:40 AM.

Details

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.

Event Timeline

jdoerfert created this revision.Nov 5 2019, 8:40 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 5 2019, 8:40 AM
Herald added a subscriber: jholewinski. · View Herald Transcript
ABataev added inline comments.Nov 5 2019, 8:57 AM
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 marked 3 inline comments as done.Nov 5 2019, 9:22 AM
jdoerfert added inline comments.
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.)
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 updated this revision to Diff 227948.Nov 5 2019, 12:38 PM
jdoerfert marked 5 inline comments as done.

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

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 marked an inline comment as done.Nov 5 2019, 1:45 PM
jdoerfert added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
4017–4018

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

ABataev added inline comments.Nov 5 2019, 1:55 PM
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 marked an inline comment as done.Nov 5 2019, 2:30 PM
jdoerfert added inline comments.
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.

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.

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 added inline comments.Nov 5 2019, 3:38 PM
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 added inline comments.
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 updated this revision to Diff 227985.Nov 5 2019, 4:55 PM

More updates

jdoerfert marked 5 inline comments as done.Nov 6 2019, 10:24 AM
jdoerfert added inline comments.
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 updated this revision to Diff 228126.Nov 6 2019, 12:56 PM
jdoerfert marked an inline comment as done.

working version

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

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 marked 2 inline comments as done.Nov 7 2019, 10:07 AM

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 updated this revision to Diff 228275.Nov 7 2019, 10:08 AM
jdoerfert marked an inline comment as done.

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

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 marked 2 inline comments as done.Nov 7 2019, 11:49 AM

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 accepted this revision.Nov 7 2019, 12:06 PM

LG with a nit

llvm/include/llvm/IR/OpenMPConstants.h
1 ↗(On Diff #228275)

Just OpenMPConstants.h?

This revision is now accepted and ready to land.Nov 7 2019, 12:06 PM
jdoerfert updated this revision to Diff 228788.Mon, Nov 11, 5:29 PM
jdoerfert marked 2 inline comments as done.

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 added inline comments.
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 marked 4 inline comments as done.Tue, Nov 26, 10:54 PM
jdoerfert added inline comments.
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.