This is an archive of the discontinued LLVM Phabricator instance.

[openmp] Base of tablegen generated OpenMP common declaration
ClosedPublic

Authored by clementval on Jun 12 2020, 6:57 AM.

Details

Summary

As discussed previously when landing patch for OpenMP in Flang, the idea is
to share common part of the OpenMP declaration between the different Frontend.
While doing this it was thought that moving to tablegen instead of Macros will also
give a cleaner and more powerful way of generating these declaration.
This first part of a future series of patches is setting up the base .td file for
DirectiveLanguage as well as the OpenMP version of it. The base file is meant to
be used by other directive language such as OpenACC.
In this first patch, the Directive and Clause enums are generated with tablegen
instead of the macros on OMPConstants.h. The next pacth will extend this
to other enum and move the Flang frontend to use it.

Diff Detail

Event Timeline

clementval created this revision.Jun 12 2020, 6:57 AM

Fix new line at tend of file

clementval added a project: Restricted Project.Jun 12 2020, 7:01 AM

@jdoerfert Some tests in clang relies on the position of the specific enum in the declaration. TableGen is sorting the records so the enum is generated in alphabetical order therefore the enum position is changed. Is it fine to update the test with smith like {{.*}} instead of the specific id?

Example of failure: Old position 9, new position 23

./llvm-project/clang/test/AST/ast-dump-openmp-target-teams-distribute-simd.c:74:16: error: CHECK-NEXT: expected string not found in input
// CHECK-NEXT: | | | | | `-OMPCaptureKindAttr {{.*}} <<invalid sloc>> Implicit 9
               ^
<stdin>:51:1: note: scanning from here
| | | | | `-OMPCaptureKindAttr 0x43ee9600 <<invalid sloc>> Implicit 23

Keep same ordering than OMPKinds.def + add dependency

Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2020, 12:07 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This is really cool :)

@jdoerfert Some tests in clang relies on the position of the specific enum in the declaration. TableGen is sorting the records so the enum is generated in alphabetical order therefore the enum position is changed. Is it fine to update the test with smith like {{.*}} instead of the specific id?

Yes, that's fine.

I left some comments and added some more reviewers.

llvm/include/llvm/Frontend/Directive/DirectiveBase.td
70

I really like we have this abstraction now and also a nice way to encode some constraints, e.g., required clauses, right away :)

llvm/include/llvm/Frontend/OpenMP/OMP.td
497

I guess we go over this in a follow up and use the requiredClauses and similar features, that seems reasonable to me.

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
50

While the MACRO MAGIC worked surprisingly well, I'm glad it is going away.

llvm/test/TableGen/directive.td
34 ↗(On Diff #270481)

How does the allowedClauses affect the result?

clementval marked 3 inline comments as done.Jun 15 2020, 6:39 AM
clementval added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMP.td
497

Yes, that was the plan. I tried to stick to the way it is done today. Then we can use the extra abstraction when more things are in place.

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
50

I hope we can replace as much as possible the MACRO MAGIC. We might have to keep some of it were there is no clear abstraction. Let's see.

llvm/test/TableGen/directive.td
34 ↗(On Diff #270481)

It does not affect the result in this case. It will be used in a next patch were with can replace the isAllowedClauseForDirective code generation with TableGen.

Avoid code relying on enum position.

Update test

Assuming this passes all the tests, it looks good to me. Let's wait a day or two for people to take a look though :)

llvm/test/TableGen/directive.td
34 ↗(On Diff #270481)

Gotcha.

Assuming this passes all the tests, it looks good to me. Let's wait a day or two for people to take a look though :)

@jdoerfert Sure let's wait a bit. I see a failure because of some clang-tidy warnings. How strongly should I fix them? I see some conflict with other code I see in TableGen.

Assuming this passes all the tests, it looks good to me. Let's wait a day or two for people to take a look though :)

@jdoerfert Sure let's wait a bit. I see a failure because of some clang-tidy warnings. How strongly should I fix them? I see some conflict with other code I see in TableGen.

Clang-tidy run by Phab is not binding but if it's reasonable modify it. Keep the TableGen style consistent.

Fix some clang-tidy warning

Assuming this passes all the tests, it looks good to me. Let's wait a day or two for people to take a look though :)

@jdoerfert Sure let's wait a bit. I see a failure because of some clang-tidy warnings. How strongly should I fix them? I see some conflict with other code I see in TableGen.

Clang-tidy run by Phab is not binding but if it's reasonable modify it. Keep the TableGen style consistent.

@jdoerfert I made some fix to fit in the TableGen style. So this is ready to land on my side.
I'm working on the next patches. One that replace the specific enums in Flang to use the one defined here. Another one that continue to replace the MACROS in OMPConstants.h/.cpp with TableGen code generation.

This patch looks like a good idea to me. Thanks.

@jdoerfert Some tests in clang relies on the position of the specific enum in the declaration. TableGen is sorting the records so the enum is generated in alphabetical order therefore the enum position is changed. Is it fine to update the test with smith like {{.*}} instead of the specific id?

Example of failure: Old position 9, new position 23

./llvm-project/clang/test/AST/ast-dump-openmp-target-teams-distribute-simd.c:74:16: error: CHECK-NEXT: expected string not found in input
// CHECK-NEXT: | | | | | `-OMPCaptureKindAttr {{.*}} <<invalid sloc>> Implicit 9
               ^
<stdin>:51:1: note: scanning from here
| | | | | `-OMPCaptureKindAttr 0x43ee9600 <<invalid sloc>> Implicit 23

This seems like a case where the dumper should be printing a name instead of a number. Given that you're touching all the tests for that anyway, I wonder how hard it would be to go ahead and fix that, perhaps in a parent patch so that this patch becomes easier to review. Ignore this suggestion if it's too much of a distraction.

clang/test/AST/ast-dump-openmp-target-parallel-for.c
105

Why 23 not {{.*}} like the others?

llvm/include/llvm/Frontend/Directive/DirectiveBase.td
23

"dialect" -> "directive language" as above?

llvm/test/TableGen/directive.td
11 ↗(On Diff #271264)

Does it make sense to go ahead and test the case where this is 0? What about testing enableBitmaskEnumInNamespace?

clementval marked an inline comment as done.

Address comment from @jdenny

clementval marked 10 inline comments as done.Jun 17 2020, 1:07 PM

@jdenny I will have a look at your suggestion to dump the name instead of the id. If it looks straight forward I will do it. Other comments were addressed. Thanks.

clang/test/AST/ast-dump-openmp-target-parallel-for.c
105

Good catch!

llvm/include/llvm/Frontend/Directive/DirectiveBase.td
23

Should be "directive language". copy-paste error.

llvm/test/TableGen/directive.td
11 ↗(On Diff #271264)

I added a test for that.

Thanks for addressing my comments. As @jdoerfert says, let's give others time to review.

llvm/test/TableGen/directive1.td
2

I realize that organizing tests into files like directive1.td and directive2.td is common, and it's probably fine when you have only two test files. However, I think this is going to become hard to navigate as the number of tests grow. Even now, I find myself wanting to run diff to understand the significance of these two files.

Can you define these two languages in the same .td file? That might make sense here given how similar these two are. Comments can then easily explain the difference between them.

Eventually, I think there should be a directive directory and its files should be named to indicate their purpose.

You don't necessarily need to address all this now, but please keep it in mind as this evolves.

clementval updated this revision to Diff 271547.EditedJun 17 2020, 6:52 PM
clementval marked 3 inline comments as done.

Add missing break + small renaming of emit function

@jdoerfert @jdenny Should we wait until Monday to go ahead with this patch? I have several other patches that will follow this one.

jdenny accepted this revision.Jun 19 2020, 9:26 AM

@jdoerfert @jdenny Should we wait until Monday to go ahead with this patch? I have several other patches that will follow this one.

One week is the usual grace period. If @jdoerfert is happy at this point, I'm happy.

This revision is now accepted and ready to land.Jun 19 2020, 9:26 AM
jdoerfert accepted this revision.Jun 19 2020, 10:04 AM

Happy. LGTM

This revision was automatically updated to reflect the committed changes.
clementval reopened this revision.Jun 22 2020, 8:59 AM

Missing dependencies

This revision is now accepted and ready to land.Jun 22 2020, 8:59 AM

Add missing dependencies

clementval requested review of this revision.Jun 22 2020, 10:13 AM

@jdoerfert @jdenny I had to add a bunch of dependencies so that the file is generated correctly when needed. Do you have any feedback on this?

My cmake skills are lacking. Why are there are so many new DEPENDS relationships where there were none before? Is it because omp_gen is generating a header file that's included (indirectly) in all those places, where apparently that sort of dependency was unusual?

Have you tried building from a new, empty build directory on your local system? Can you reproduce the CI fails without this fixup?

My cmake skills are lacking. Why are there are so many new DEPENDS relationships where there were none before? Is it because omp_gen is generating a header file that's included (indirectly) in all those places, where apparently that sort of dependency was unusual?

Have you tried building from a new, empty build directory on your local system? Can you reproduce the CI fails without this fixup?

Yeah they are needed because of the indirect inclusion of the generated file. I could reproduce the failure from an empty build directory. This leads to the addition of the different depends in cmake files.

jdenny accepted this revision.Jun 22 2020, 12:54 PM

If you feel reasonably confident that each new DEPENDS is needed, then this LGTM. Otherwise, give it a day to see if anyone with stronger cmake skills than me has a comment.

This revision is now accepted and ready to land.Jun 22 2020, 12:54 PM
thakis added a subscriber: thakis.Jun 23 2020, 7:43 AM
thakis added inline comments.
llvm/include/llvm/Frontend/OpenMP/CMakeLists.txt
3

All other tblgen outputs are called .inc, not .h.inc. Any reason this one's different?

This revision was automatically updated to reflect the committed changes.
jdoerfert added inline comments.Jun 23 2020, 8:10 AM
llvm/include/llvm/Frontend/OpenMP/CMakeLists.txt
3
clementval marked 2 inline comments as done.Jun 23 2020, 8:34 AM
clementval added inline comments.
llvm/include/llvm/Frontend/OpenMP/CMakeLists.txt
3

There is a .cpp.inc coming in a following patch.

thakis added inline comments.Jun 23 2020, 10:33 AM
llvm/include/llvm/Frontend/OpenMP/CMakeLists.txt
3

...why would you want to include a cpp file?

If it's for definitions of generated functions, I think the usual pattern is to put that in the .inc too behind a define and define that in one cpp file that includes the .inc. (Examples: GET_DAGISEL_BODY, GET_INSTRINFO_MC_DESC, PRINT_ALIAS_INSTR etc -- rg -B5 '#include.*\.inc' clang llvm shows many examples).

clementval marked 3 inline comments as done.Jun 23 2020, 11:07 AM
clementval added inline comments.
llvm/include/llvm/Frontend/OpenMP/CMakeLists.txt
3

Yeah this was the idea. I was following the same pattern as MLIR use of TableGen. I'm happy with a single .inc file with define. If you don't mind I'll update the filename in the next patch since this one has landed already.

FYI MLIR TableGen .cpp.inc -> https://github.com/llvm/llvm-project/blob/master/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp#L42

vdmitrie added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
197

Should this be a comparison against llvm::omp::Directive_enumSize rather than OMPD_unknown?

And there is an assertion in file clang/lib/Basic/OpenMPKinds.cpp
that I guess needs to be updated the same way:
void clang::getOpenMPCaptureRegions(

  SmallVectorImpl<OpenMPDirectiveKind> &CaptureRegions,
  OpenMPDirectiveKind DKind) {
assert(DKind <= OMPD_unknown);
clementval marked 3 inline comments as done.Jun 24 2020, 6:20 PM
clementval added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
197

Good catch thx. I just created a patch to fix this D82518

This seems to cause build errors sometimes when buildling libclang. Maybe there are more missing dependencies? Example failure: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8876400305251533648/+/steps/gclient_runhooks/0/stdout

clementval marked an inline comment as done.Jun 26 2020, 5:18 PM

This seems to cause build errors sometimes when buildling libclang. Maybe there are more missing dependencies? Example failure: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8876400305251533648/+/steps/gclient_runhooks/0/stdout

Could this patch solve your problem? https://reviews.llvm.org/D82659