-
Notifications
You must be signed in to change notification settings - Fork 79
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
Migrating Target Attributes to IRModule #29
Conversation
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.
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.
@jroesch @mbs-octoml @tkonolige would be good to get your thoughts. overall I agree we need to move these attributes out of Target. I'm not sure they should go on the IRModule--rather my initial thoughts on this was that we should just create a separate RuntimeConfig option and pass that to |
I think this is a good and necessary change. Thanks! I agree with Andrew that it would be better to keep these parameters as a separate object. I think this makes a more consistent interface. We are all ready passing targets separately from the module, so it makes more logical sense to keep "RuntimeConfig" (or whatever you want to call it) separate. On the other hand, I'm also not opposed to moving target and "RuntimeConfig" into the IRModule. Passing around all these separate but related objects can be a pain and I think we are moving towards bundling them in the IRModule. |
+1 overall. We've been thinking along the same lines for replacing the explicit TargetMap (and it's convention for designating the 'default' target) and the 'host target' + 'device target' convention with a single datastructure describing all the targets which are significant for compilation and which are to be used as defaults for the different situations (for shape computation, for residual loops after offloading to prims, for generic relay computation). I'm trying to write that up as an RFC and will ping y'all asap. I'm 80% on the just-put-it-in-the-IRModule camp since that is the one datastructure we are already threading all the way through. But we also make very heavy use of implicit global stacks (eg pass contexts), as well as pass-everywhere arguments (eg existing targets, target pairs and target maps). Perhaps that's the only contentious thing we need to resolve here? |
@mbs-octoml my only worry about putting things in an IRModule is that it's not a very user-friendly thing to do, and these are all user-facing options. for that reason i think the separate object makes sense and we can migrate the config into IRModule if we wish to place it there internally within the compiler. |
Thanks for the feedback @junrushao1994, @areusch, @tkonolige and @mbs-octoml, this is one of those design decisions where I was 50/50 on whether to keep tvm.relay.build(
ir_module,
target,
target_host=target,
executor=Executor({
"kind": "aot",
"unpacked-api": False
}),
runtime=Runtime({
"kind": "c",
"link-params": False
}),
params=params,
mod_name="the_ultimate_cat_spotter",
) Hopefully this addresses everyone's concerns 😸 |
[sorry, finger flub, sent too early] I think I'm suggesting we need to collect the executor, runtime and various targets under a single CompilerConfig structure and bind that into the IRModule. That means it's just a Then I think I'll slightly pivot my target/device RFC into a 'handling multiple targets' RFC, and use the ComplierConfig as the carrier for what would replace the existing TargetMap and various defaulting conventions. Am I making sense? |
Hi @mbs-octoml, apologies, I'm not sure how grouping them together on the |
@Mousius, just a gut feeling that we should try to group statically known and related things together as an ObjectRef/Node pair rather than rely on string attribute names and dynamic type checking. After fiddling a bit with multi-target handling I think I'll want a few extra fields for capturing the default targets for primitives and residual relay/shape computation, but that's just two more fields which similarly can go into the mix of attributes. We don't have to sweat it now, do what feels right to you. |
@mbs-octoml I think we'll have more detail in each attribute as part of the various configurations, as you'll have: SetAttr<kRuntime>(Runtime(...));
SetAttr<kTarget>(Target(...)); And as such I think I'd prefer to leave it as-is. @junrushao1994 / @areusch / @tkonolige are there further concerns before we merge this? |
Overall this seems like a good change! Right now, the PassContext has some module-level configuration options, but it also has options that are extracted from the PassContext within passes and modifies the behavior of those passes. I think that we should at least move the module-level configuration options into the IRModule attributes, but I'm not sure about what we should do with the other options. Another question is whether we want to distinguish between attributes / config options that are pass-invariant and attributes that passes can change. This would make it easier to make sure that the pass-invariant ones are being propagated properly. |
LGTM |
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.
LGTM
Hi @electriclilies, Following up on your questions 😸 Overall I think we're thinking the same things in terms of direction of travel!
I completely agree, I think the direction of travel should be towards removing it or integrating it with the
This would be an RFC of interest to me, moving them into attributes and only passing the IRModule seems sensible - the
Agree, we should probably try to freeze attributes which aren't meant to change (hopefully most of them) and that would hopefully make things simpler for everyone. Unsure if TVM has support for this in |
@mbs-octoml yeah this does make sense to me. sorry for the delay in reply here. |
…d registries This is the underpinning Executor/Runtime objects for apache/tvm-rfcs#29, this doesn't change the wiring as yet since that's a pretty big change in itself. Most of this patch is TVM boilerplate, which I've tried to minimize - there's maybe some future work to decide how to more easily define some of these things.
…d registries This is the underpinning Executor/Runtime objects for apache/tvm-rfcs#29, this doesn't change the wiring as yet since that's a pretty big change in itself. Most of this patch is TVM boilerplate, which I've tried to minimize - there's maybe some future work to decide how to more easily define some of these things.
…d registries This is the underpinning Executor/Runtime objects for apache/tvm-rfcs#29, this doesn't change the wiring as yet since that's a pretty big change in itself. Most of this patch is TVM boilerplate, which I've tried to minimize - there's maybe some future work to decide how to more easily define some of these things.
…d registries This is the underpinning Executor/Runtime objects for apache/tvm-rfcs#29, this doesn't change the wiring as yet since that's a pretty big change in itself. Most of this patch is TVM boilerplate, which I've tried to minimize - there's maybe some future work to decide how to more easily define some of these things.
…d registries This is the underpinning Executor/Runtime objects for apache/tvm-rfcs#29, this doesn't change the wiring as yet since that's a pretty big change in itself. Most of this patch is TVM boilerplate, which I've tried to minimize - there's maybe some future work to decide how to more easily define some of these things.
…d registries This is the underpinning Executor/Runtime objects for apache/tvm-rfcs#29, this doesn't change the wiring as yet since that's a pretty big change in itself. Most of this patch is TVM boilerplate, which I've tried to minimize - there's maybe some future work to decide how to more easily define some of these things.
…d registries (#9246) This is the underpinning Executor/Runtime objects for apache/tvm-rfcs#29, this doesn't change the wiring as yet since that's a pretty big change in itself. Most of this patch is TVM boilerplate, which I've tried to minimize - there's maybe some future work to decide how to more easily define some of these things.
…d registries (apache#9246) This is the underpinning Executor/Runtime objects for apache/tvm-rfcs#29, this doesn't change the wiring as yet since that's a pretty big change in itself. Most of this patch is TVM boilerplate, which I've tried to minimize - there's maybe some future work to decide how to more easily define some of these things.
…d registries (apache#9246) This is the underpinning Executor/Runtime objects for apache/tvm-rfcs#29, this doesn't change the wiring as yet since that's a pretty big change in itself. Most of this patch is TVM boilerplate, which I've tried to minimize - there's maybe some future work to decide how to more easily define some of these things.
No description provided.