-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra #5484
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,10 @@ | |
__pycache__/ | ||
*.py[cod] | ||
*$py.class | ||
|
||
*.S | ||
# C extensions | ||
*.so | ||
*.ll | ||
|
||
# Distribution / packaging | ||
.Python | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,7 +90,8 @@ def get_target_triple(): | |
def cross_compiler(compile_func, | ||
options=None, | ||
output_format=None, | ||
get_target_triple=None): | ||
get_target_triple=None, | ||
add_files=None): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a little bit of a weird place (compiler configuration time) to add extra files (I.e. invocation configuration time). why can't this be pushed to the invocation of fcompile? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed it is an API tradeoff. The main advantage of functor decorator style is that it is composable. For example, we could pass use the following to attach minserver to the ndk compiler. mod.export_library("xyz", tvm.rpc.with_minrpc(cc.ndk)) Another reason is that in the case of minrpc, we need to attach a special property(need_system_lib) to tell the export_library to check for the We might need a bit more thoughts to make the alternative API(as follows) work. mod.export_library("xyz", cc.ndk, libs=[tvm.rpc.minrpc()]) Finally, such functor decorator is easier to work with in the AutoTVM(we just have to pass the decorated version as build_func, instead of passing an additional argument) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not very attached to the argument though. Pehaps changig to something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems like based on how this is used now, the problem is that we are trying to make export_library build a binary in a few small cases (but, which will become increasingly common as µTVM is more integrated). i think it's reasonable for export_library to control the compiler invocation up to the point of generating a .o. beyond that ldflags and additional statically-linked dependencies (which will have their own cflags and compiler toolchains) seem like a bit much of an API abuse. maybe this will be better addressed by the compilation and linking work i'm doing? if so, is there an intermediate hack we can do that avoids adding to this API (if you agree that's the right direction to go)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is mainly a difference in API choices.
My take is that A1 is as powerful as A0, and the complexity of compiler customization goes to the construction process of fcompile. It also have the advantage of moving customization to a single place (fcompile) so that we don't have to worry about customizing the export_library step in autotvm and downstream toolchains that uses export_library. We can certainly de-couple the APIs in A1. Right now compilation and linking are coupled, and a more powerful way to construct A1 might be as follows, and we can work toward that direction. fcompile = attach_linker_opts(get_compiler()) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, I think de-coupling the APIs in A1 makes sense. if we do that though, then only autotvm (and other user scripts and any µTVM veneers) need to care about linker flags and linker scripts, correct? we can remove that complexity from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think your understanding is correct |
||
"""Create a cross compiler function by specializing compile_func with options. | ||
|
||
This function can be used to construct compile functions that | ||
|
@@ -111,6 +112,10 @@ def cross_compiler(compile_func, | |
get_target_triple: Optional[Callable] | ||
Function that can target triple according to dumpmachine option of compiler. | ||
|
||
add_files: Optional[List[str]] | ||
List of paths to additional object, source, library files | ||
to pass as part of the compilation. | ||
|
||
Returns | ||
------- | ||
fcompile : Callable[[str, str, Optional[str]], None] | ||
|
@@ -133,6 +138,7 @@ def cross_compiler(compile_func, | |
""" | ||
base_options = [] if options is None else options | ||
kwargs = {} | ||
add_files = [] if add_files is None else add_files | ||
|
||
# handle case where compile_func is the name of the cc | ||
if isinstance(compile_func, str): | ||
|
@@ -144,7 +150,7 @@ def _fcompile(outputs, objects, options=None): | |
all_options = base_options | ||
if options is not None: | ||
all_options += options | ||
compile_func(outputs, objects, options=all_options, **kwargs) | ||
compile_func(outputs, objects + add_files, options=all_options, **kwargs) | ||
|
||
if not output_format and hasattr(compile_func, "output_format"): | ||
output_format = compile_func.output_format | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to ignore assembly files generated in commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok. I guess this means we can't check in assembly without removing it, but we can do that later too.