[TVMC] add the support of the cross compiler options by vinceab · Pull Request #7922 · apache/tvm

vinceab · 2021-04-26T08:17:18Z

Add the possibility to provide the cross compiler options when using the
tvmc compile functionality.
With some cross compiler toolchains --sysroot option (at least) need to be defined.

@leandron

leandron

The change looks OK to me. Apart from my comments below, is there a chance to add a test case for this PR?

leandron · 2021-04-26T09:01:31Z

    cross : str or callable object, optional
        Function that performs the actual compilation
-
+    cross_options : sst of cross compilation options


Modifying it to comply with Numpy docstring format.

Suggested change

cross_options : sst of cross compilation options

cross_options : str, optional

Command line options to be passed to the cross compiler.

leandron · 2021-04-26T09:04:33Z

    else:
        logger.debug("exporting library to %s , using cross compiler %s", path_lib, cross)
-        lib.export_library(path_lib, cc.cross_compiler(cross))
+        lib.export_library(path_lib, cc.cross_compiler(cross, options=cross_options.split(' ')))


Maybe it makes sense to add the command line options to the debug message on the line above?

leandron · 2021-04-26T09:07:22Z

+def save_module(module_path, graph, lib, params, cross=None,
+                cross_options=None):


Suggested change

def save_module(module_path, graph, lib, params, cross=None,

cross_options=None):

def save_module(module_path, graph, lib, params, cross=None, cross_options=None):

vinceab · 2021-04-26T09:23:51Z

The change looks OK to me. Apart from my comments below, is there a chance to add a test case for this PR?

I will do the update of the patch.
Regarding the test of this PR, I have tested it with the STM32MP157 board. In which location I can add a test case?

Vincent

leandron · 2021-04-28T15:29:30Z

The change looks OK to me. Apart from my comments below, is there a chance to add a test case for this PR?

I will do the update of the patch.
Regarding the test of this PR, I have tested it with the STM32MP157 board. In which location I can add a test case?

Vincent

I was referring to unit tests, just to try validate the API changes. The unit tests are currently hosted at - https://github.com/apache/tvm/tree/main/tests/python/driver/tvmc

vinceab · 2021-05-05T10:26:36Z

The change looks OK to me. Apart from my comments below, is there a chance to add a test case for this PR?

I will do the update of the patch.
Regarding the test of this PR, I have tested it with the STM32MP157 board. In which location I can add a test case?
Vincent

I was referring to unit tests, just to try validate the API changes. The unit tests are currently hosted at - https://github.com/apache/tvm/tree/main/tests/python/driver/tvmc

@leandron how do you execute the tests?
On tvm main branch, I tries test_cross_compile_aarch64_tflite_module function and it fails with this error:
assert llvm_id != 0, "%s is not an LLVM intrinsic" % name
AssertionError: llvm.aarch64.neon.umull is not an LLVM intrinsic

leandron · 2021-05-11T16:19:53Z

The change looks OK to me. Apart from my comments below, is there a chance to add a test case for this PR?

I will do the update of the patch.
Regarding the test of this PR, I have tested it with the STM32MP157 board. In which location I can add a test case?
Vincent

I was referring to unit tests, just to try validate the API changes. The unit tests are currently hosted at - https://github.com/apache/tvm/tree/main/tests/python/driver/tvmc

@leandron how do you execute the tests?
On tvm main branch, I tries test_cross_compile_aarch64_tflite_module function and it fails with this error:
assert llvm_id != 0, "%s is not an LLVM intrinsic" % name
AssertionError: llvm.aarch64.neon.umull is not an LLVM intrinsic

Hi, sorry for the delay. I don't think this error is related to tvmc specifically. It probably has something to do with the way your TVM was built. Which LLVM version are you linking TVM with?

vinceab · 2021-05-12T08:22:01Z

@leandron the LLVM version I use is the v6.0.0.

leandron · 2021-05-12T08:50:30Z

@leandron the LLVM version I use is the v6.0.0.

I see. I appreciate the docs mention "LLVM 4.0 or higher is needed for build with LLVM. Note that version of LLVM from default apt may lower than 4.0.", but I don't think we test with anything < LLVM 9. Probably something to investigate in a separate issue.

Is that possible at all for your to try with some newer LLVM, like LLVM 11, which is the one we use in most of the CI jobs here?

vinceab · 2021-05-12T09:51:42Z

@leandron the LLVM version I use is the v6.0.0.

I see. I appreciate the docs mention "LLVM 4.0 or higher is needed for build with LLVM. Note that version of LLVM from default apt may lower than 4.0.", but I don't think we test with anything < LLVM 9. Probably something to investigate in a separate issue.

Is that possible at all for your to try with some newer LLVM, like LLVM 11, which is the one we use in most of the CI jobs here?

I just tested with LLVM v10 and it seems to work.
Base on this I will propose an extension of the tests to include cross compiler options

vinceab · 2021-05-19T10:37:39Z

@leandron

I have rebase the patch on the main branch and add tests.

leandron · 2021-05-19T15:53:13Z

@leandron

I have rebase the patch on the main branch and add tests.

I think the linter is complaining about formatting on your patch (the log is here). You can check some command lines to run the exact same linter locally on your machine here: https://tvm.apache.org/docs/contribute/pull_request.html#submit-a-pull-request

leandron

LGTM, pending on formatting/linter fixes.

Add the possibility to provide the cross compiler options when using the tvmc compile functionality. With some cross compiler, toolchains --sysroot option (at least) need to be defined. tvmc/test_compile.py as been updated to introduce simple tests to validate the cross options functionnality. Signed-off-by: Vincent ABRIOU <vincent.abriou@st.com>

vinceab · 2021-05-20T14:24:44Z

@leandron formatting/linter fixed

leandron

LGTM

leandron · 2021-05-21T12:22:49Z

Thanks @vinceab!

Add the possibility to provide the cross compiler options when using the tvmc compile functionality. With some cross compiler, toolchains --sysroot option (at least) need to be defined. tvmc/test_compile.py as been updated to introduce simple tests to validate the cross options functionnality. Signed-off-by: Vincent ABRIOU <vincent.abriou@st.com>

leandron reviewed Apr 26, 2021

View reviewed changes

vinceab force-pushed the tvmc_cross_compiler_options branch from 9e6dcf3 to a06f7bf Compare May 19, 2021 10:35

leandron reviewed May 19, 2021

View reviewed changes

vinceab force-pushed the tvmc_cross_compiler_options branch from a06f7bf to 189f9bb Compare May 20, 2021 14:22

leandron self-assigned this May 21, 2021

leandron approved these changes May 21, 2021

View reviewed changes

leandron merged commit 5a7c081 into apache:main May 21, 2021

junrushao mentioned this pull request Nov 1, 2021

Apache TVM v0.8 Release Note Candidate #9416

Closed

	cross_options : sst of cross compilation options
	cross_options : str, optional
	Command line options to be passed to the cross compiler.

		def save_module(module_path, graph, lib, params, cross=None,
		cross_options=None):

Conversation

vinceab commented Apr 26, 2021

Uh oh!

leandron left a comment

Choose a reason for hiding this comment

Uh oh!

leandron Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

leandron Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

leandron Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

vinceab commented Apr 26, 2021

Uh oh!

leandron commented Apr 28, 2021

Uh oh!

vinceab commented May 5, 2021

Uh oh!

leandron commented May 11, 2021

Uh oh!

vinceab commented May 12, 2021

Uh oh!

leandron commented May 12, 2021

Uh oh!

vinceab commented May 12, 2021

Uh oh!

vinceab commented May 19, 2021

Uh oh!

leandron commented May 19, 2021

Uh oh!

leandron left a comment

Choose a reason for hiding this comment

Uh oh!

vinceab commented May 20, 2021

Uh oh!

leandron left a comment

Choose a reason for hiding this comment

Uh oh!

leandron commented May 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants