Page MenuHomePhabricator

[openacc][openmp] Reduce number of generated file and prefer inclusion of .inc
ClosedPublic

Authored by clementval on Dec 18 2020, 12:27 PM.

Details

Summary

Follow up from D92955 and D83636. This patch makes the base cpp files
OMP.cpp and ACC.cpp normal files and they now include the XXX.inc file
generated by tablegen. This reduces the number of file generated by the
DirectiveEmitter backend and makes it closer to the proposal in D83636.

Diff Detail

Event Timeline

clementval created this revision.Dec 18 2020, 12:27 PM
clementval requested review of this revision.Dec 18 2020, 12:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 18 2020, 12:27 PM

@thakis If it still makes sense to make the directive TableGen backend closer to your initial proposal (D83636) please have a look. Otherwise I'll abandon this revision.
@jdoerfert

Not generating a translation unit directly but including the generated part is great.

However, emitting everything into a single file and selecting using preprocessor directives doesn't seem what other tablegen targets are doing. E.g. for intrinsics there is IntrinsicImpl.inc, IntrinsicEnums.inc. Clang has Attrs.inc, AttrImpl.inc, AttrVisitor.inc, etc.

However, emitting everything into a single file and selecting using preprocessor directives doesn't seem what other tablegen targets are doing. E.g. for intrinsics there is IntrinsicImpl.inc, IntrinsicEnums.inc. Clang has Attrs.inc, AttrImpl.inc, AttrVisitor.inc, etc.

This is what MLIR is doing with all its TableGen related code generation. Since we are generating lots of different code block, the number of files would probably explode.

The reason why MLIR does not split declaration and implementation-generation might be that nobody yet wanted to invest the work to split them. While I would not request splitting tablegen-generated files into separate files, fusing feels like a step backwards. If only implementation details change, with a merged output everything that includes OMP.h must be recompiled as well. I don't see how a separation into declaration and implementation causes the number of files to explode.

The reason why MLIR does not split declaration and implementation-generation might be that nobody yet wanted to invest the work to split them. While I would not request splitting tablegen-generated files into separate files, fusing feels like a step backwards. If only implementation details change, with a merged output everything that includes OMP.h must be recompiled as well. I don't see how a separation into declaration and implementation causes the number of files to explode.

We are not fusing header and implementation. We had initially 3 files and after comments from a previous patch it was recommended to reduce that to two files. So after this patch you will have one header file and the implementation with the different code block.

OK, I found the header include is generated in CMakeLists.txt in include/llvm/Frontend. This patch removes the "-gen-directive-impl" target and integrates it into "-gen-directive-gen". IMHO "-gen-directive-impl" is the better name for for the implementation file, following the scheme set by "AttrImpl.inc" and "IntrinsicImpl.inc".

Sorry for me not noticing that there have been 3 tablegen targets per td.

Meinersbur accepted this revision.Tue, Mar 16, 3:08 PM

I'd prefer using the name "impl" for the implementation, but my LGTM does not depend on it.

llvm/utils/TableGen/TableGen.cpp
141

What does "part" refer to?

This revision is now accepted and ready to land.Tue, Mar 16, 3:08 PM

I'd prefer using the name "impl" for the implementation, but my LGTM does not depend on it.

Thanks for the review.

I'm fine with changing the current name to impl. @kiranchandramohan What do you think about it?

llvm/utils/TableGen/TableGen.cpp
141

It refers to various code "part" needed in clang and flang and libFrontend. Not sure if we can be more precise here.

Meinersbur added inline comments.Wed, Mar 17, 5:50 AM
llvm/utils/TableGen/TableGen.cpp
141

"part" seems to refer to -gen-directive-impl, as in "the other part of the implementation". Again, I suggest to keep the gen-directive-impl flag instead.

Rebase and address review comment

This revision was landed with ongoing or failed builds.Tue, Mar 23, 6:17 AM
This revision was automatically updated to reflect the committed changes.

Great change, thanks for doing it!

Can we fold the .h.inc files into the .inc files too? I haven't seen .h.inc files elsewhere in the build.

Great change, thanks for doing it!

Can we fold the .h.inc files into the .inc files too? I haven't seen .h.inc files elsewhere in the build.

We can do it as well if necessary. MLIR has a lot of .h.inc.