Add runtime.ModuleGetFormat method enabling export of BYOC generated sources which require a .cpp/.cc file extension #9243

PhilippvK · 2021-10-11T08:41:26Z

Motivation

I ran into the issue that I need to use C++ features in a BYOC generated kernel which is tricky to achieve because those are typically exported as .c files.

I am aware of the possibility to force gcc/g++ to threat all input files as C++ but this solves the issue only partially and is not really intuitive. Renaming the file names e.g. changing the file extension manually after the export via MLF is if also a workaround which does not work out for me.

Solution(s):

I came up with two rather simple solutions to solutions to solve the explained limitation in the ability to choose the correct file extension for kernel files written by a custom codegen and implemented the first one in this PR because it should add minimal complexity to the TVM codebase which might be desirable:

Each CSourceModuleNode already has a format field which can be set in the constructor. Currently it can not be accessed by other classes but this can be easily solved by adding a getter and the respective python hooks for that property. This information can then be accessed by i.e. model_library_format.py to determine the file extension for the exported kernel. It would also be possible to use this interface for other types of modules in the future.
Introduce a new class CppSourceModuleNode which is essentially a copy of CSourceModule with the only difference being the the the module_key which may then be used to choose a file extension in the aforementioned python functions. This sounds like much redundant code to me and also has less flexibility compared to option one as we would have to stick with either a .cc or a .cpp file extension for every modules of that type.

Open questions/thoughts:

I realized that the module format cc is currently already used by the CSourceCrtMetadataModuleNode declared in source_module.cc:

auto n = make_object<CSourceCrtMetadataModuleNode>(func_names, "cc", target, metadata);

This does not really make sense to me and especially makes some AoT and Model Library Format related pytests fail because the metadata module now being exported as .cc instead of .c as explaind the the previous paragraph. Let me know if I should add the fix these Makefiles to this PR and keep the module format for the CSourceCrtMetadataModuleNode defined as cc or if should work around this issue instead by changing the module format of that file to c?
I did not yet come up with new test cases which exercise the introduced changes to the MLF export as this would very likely result in a need to add a new dummy BYOC module which uses the cc format instead. During development I just patched the example codegen_c to export C++ kernels instead which worked fine,

I decided against an RFC for this discussion as only a couple of lines are added to the TVM codebase to realize this feature. Let me know if I should make this a (Pre-)RFC instead. Otherwise I would be happy for any feedback and answers to the remaining questions introduced in the last section.

CC @areusch @Mousius @giuseros @manupa-arm

…y fix failing tests I realized that the module format `cc` is currently already used by the `CSourceCrtMetadataModuleNode` declared in `src/target/source/source_module.cc`. This needs to be discussed first to decide if either the module_key should be changed or the test cases expecting the systemlib kernel (e.g. `default_lib0.c`) to have a `.c` extension.

…extensions AOT: Add c++ support to aot_test.mk AOT: Add c++ support to corstone300.mk

PhilippvK · 2021-10-11T12:18:32Z

Ethosu and CMSIS-NN related builds seem to fail in CI, I will look into this and follow up with a fix for that.

huajsj

Not see any UT logic, may need a test to explain the use case.

python/tvm/runtime/module.py

huajsj · 2021-10-13T04:07:28Z

python/tvm/runtime/module.py

            if fcompile is not None and hasattr(fcompile, "object_format"):
                if module.type_key == "c":
-                    object_format = "c"
+                    assert module.format in ["c", "cc", "cpp"]


assert with error message?

include/tvm/runtime/module.h

PhilippvK · 2021-10-14T10:40:40Z

Thank you for your review @huajsj. I will try to apply those patches soon.

Regarding unit tests, to did not want to introduce a new but otherwise useless BYOC codegen just for testing purposes. I will try to come up with something as simple as possible to test to changes to the SourceModule export. Eventuelly I will also figure out how include the special case nvcc -> cu into the tests.

PhilippvK · 2021-10-14T10:43:36Z

To fix the CI run i will very likely need to rebase on top of main. This will eventually screw up open discussions in this PR. Please let me know if should just merge main into this branch instead to prevent that.

areusch · 2021-10-15T15:16:03Z

@PhilippvK yeah feel free to merge main. The github workflow is really confusing to me--basically what I do (mostly) is rebase and force-push until my PRs receive a comment; the minute they receive a comment, I merge instead. Then GitHub isn't allowed to garbage collect the old pushes.

PhilippvK · 2021-10-21T13:16:17Z

Sorry for the CI bugs. I am working on them.

PhilippvK · 2021-11-05T10:20:59Z

Sorry for not following up on this. While the CI bugs have been fixed in the meantime, there are still 3 points open for discussion:

Currently the function ModuleNode::GetFormat is defined in src/runtime/module.cc but each inherited class need to implement the trivial method by itself. The reason for this decision is, that not every module is supposed to have a format. I first thought only CSourceModuleNode and CSourceCrtMetadataModuleNode needs to be updated but it seems like runtime modules such as CMSISNNModuleNode and EthosUModuleNode are directly based on 'runtime::ModuleNode' instead of 'CSourceModuleNode' leading to a need to implement a 'GetFormat' function for them as well. Do you have an advice on how to avoid this?
Coming up with proper unit tests for this feature: I think there are two parts which shall be tested individually:
- The handling of the format argument in the runtime.CSourceModuleCreate function invoced by severel BYOC kernels as well as the 'GetFormat()' function which should return the specified value.
- Export of C++ kernels with the required file extension i.e. via the Model Library Format. This should also include tests ensuring that special cases (e.g. nvcc which has the .cu extension hardcoded) do not break.
As mentioned above, i was wondering, why the CSourceCrtMetadataModuleNode is using the format cc instead of c, which would result in a change of the file extension for this source file to tvmgen_default_lib0.cc instead of tvmgen_default_lib0.c and therefore likely break some Makefile-Setups where kernel filenames are hardcoded.

areusch

@PhilippvK sorry I missed this! one question on the approach here.

areusch · 2021-11-10T03:50:29Z

python/tvm/runtime/module.py

+        """Get the format of the module."""
+        return _ffi_api.ModuleGetFormat(self)
+
    def get_source(self, fmt=""):


i think Module can sometimes have multiple formats (e.g. CUDA modules do this I believe). what about leveraging this and modifying export_module to try with fmt="c" here and if not also try fmt="cc"?

PhilippvK · 2021-11-26T12:07:51Z

@areusch Sorry but I do not really understand what you mean. Could you tell me how this helps solving my mentioned "issues"?

masahi · 2022-01-09T23:10:00Z

@PhilippvK please resolve the conflict.

…sources which require a .cpp/.cc file extension (apache#9243) * Allow export of C++ kernels using correct file extension * [WIP] Set module_key=c for CSourceCrtMetadataModuleNode to temporarily fix failing tests I realized that the module format `cc` is currently already used by the `CSourceCrtMetadataModuleNode` declared in `src/target/source/source_module.cc`. This needs to be discussed first to decide if either the module_key should be changed or the test cases expecting the systemlib kernel (e.g. `default_lib0.c`) to have a `.c` extension. * Update Makefiles used by tests/python/relay/aot/ to support C++ file extensions AOT: Add c++ support to aot_test.mk AOT: Add c++ support to corstone300.mk * Add missing definition of GetFormat to cmsisnn and ethosn codegens (WIP) * Resolve PR comments * lint python/tvm/runtime/module.py * fix EthosUModuleNode for CI * Fix: detect empty module.format * Add error message to assertion * Lint python/tvm/runtime/module.py

PhilippvK added 3 commits October 11, 2021 10:18

Allow export of C++ kernels using correct file extension

51f58a9

Update Makefiles used by tests/python/relay/aot/ to support C++ file …

2024bc6

…extensions AOT: Add c++ support to aot_test.mk AOT: Add c++ support to corstone300.mk

PhilippvK requested review from ZihengJiang, areusch, comaniac, jroesch, junrushao, kazum, kparzysz-quic, liangfu, manupak, masahi, merrymercy, tmoreau89, tqchen, vinx13 and yzhliu as code owners October 11, 2021 08:41

huajsj reviewed Oct 13, 2021

View reviewed changes

Add missing definition of GetFormat to cmsisnn and ethosn codegens (WIP)

fdfc59e

PhilippvK added 2 commits October 21, 2021 07:30

Resolve PR comments

0e7338d

Merge remote-tracking branch 'upstream/main' into export-cpp-sources

2c4170e

PhilippvK requested review from mbaret, trevor-m and zhiics as code owners October 21, 2021 05:39

lint python/tvm/runtime/module.py

5ffbde0

PhilippvK added 2 commits October 21, 2021 08:35

fix EthosUModuleNode for CI

b6100c3

Fix: detect empty module.format

4158079

Merge remote-tracking branch 'upstream/main' into export-cpp-sources

53fce80

areusch reviewed Nov 10, 2021

View reviewed changes

PhilippvK added 2 commits January 10, 2022 11:25

Add error message to assertion

2daf5bb

Merge branch 'main' into export-cpp-sources

1ec0c27

PhilippvK requested a review from icemelon as a code owner January 10, 2022 10:50

Lint python/tvm/runtime/module.py

b1d9a5e

jroesch assigned masahi and areusch Jan 19, 2022

masahi approved these changes Jan 19, 2022

View reviewed changes

masahi merged commit cc5382e into apache:main Jan 19, 2022

masahi mentioned this pull request Jan 24, 2022

[CUTLASS] Profile only the largest-possible alignment by default #10036

Merged

PhilippvK deleted the export-cpp-sources branch January 23, 2024 16:59

Add runtime.ModuleGetFormat method enabling export of BYOC generated sources which require a .cpp/.cc file extension #9243

Add runtime.ModuleGetFormat method enabling export of BYOC generated sources which require a .cpp/.cc file extension #9243

Uh oh!

Conversation

PhilippvK commented Oct 11, 2021

Motivation

Solution(s):

Open questions/thoughts:

Uh oh!

PhilippvK commented Oct 11, 2021

Uh oh!

huajsj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

huajsj Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PhilippvK commented Oct 14, 2021

Uh oh!

PhilippvK commented Oct 14, 2021

Uh oh!

areusch commented Oct 15, 2021

Uh oh!

PhilippvK commented Oct 21, 2021

Uh oh!

PhilippvK commented Nov 5, 2021

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

areusch Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

PhilippvK commented Nov 26, 2021

Uh oh!

masahi commented Jan 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants