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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
@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 is really cool :)
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? |
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. |
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. |
@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.
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? |
@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. |
@jdoerfert @jdenny Should we wait until Monday to go ahead with this patch? I have several other patches that will follow this one.
@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?
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.
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.
llvm/include/llvm/Frontend/OpenMP/CMakeLists.txt | ||
---|---|---|
3 | All other tblgen outputs are called .inc, not .h.inc. Any reason this one's different? |
llvm/include/llvm/Frontend/OpenMP/CMakeLists.txt | ||
---|---|---|
3 |
llvm/include/llvm/Frontend/OpenMP/CMakeLists.txt | ||
---|---|---|
3 | There is a .cpp.inc coming in a following patch. |
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). |
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 |
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 SmallVectorImpl<OpenMPDirectiveKind> &CaptureRegions, OpenMPDirectiveKind DKind) { assert(DKind <= OMPD_unknown); |
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
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(