-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[Relay] [Virtual Device] Store function result virtual device in virtual_device_ field #9848
[Relay] [Virtual Device] Store function result virtual device in virtual_device_ field #9848
Conversation
928464b
to
800db32
Compare
@mbs-octoml @mikepapadim @comaniac This is ready for review! |
e33c747
to
00ca9ad
Compare
ae49067
to
1205fd0
Compare
Sorry, mistyped a bit above-- because the virtual device field doesn't store the virtual device for all expression types right now, What will be true is that |
Thanks @electriclilies let me try to dissect what you said:
Note that C0 and C1 are slightly different. I believe you are talking about C0, but seems there is also a slightly indication of C1(which will indeed imply storing something on the function node(e.g. like the annotation of return type) ). In a fully heterogenous setting, the input and return value's device might be different. In such cases, "execute on" can become ambiguous and differ from the "result of return value". The current change proposes to use These are high-level discussions for your information and hope that they will help clarifying the semantics of virtual device field. They are not asks about specific kind of implementation. So feel free to take the information and make a call, while documenting the rationales that might help future refactors. |
Hi @tqchen, yes things do indeed get subtle on function types! I tried to capture that in the header mega-comment in device_planner.cc. Inside the device planner we can talk about the 'device domain' for any expression, including those of function type which describes the device domains for the args and result separately. Ie the domain for planning is currently But when it comes time to the virtual_device field I'm not convinced it's worth exposing this higher-order domain machinery, so the field just has plain-old first order VirtualDevice. So now we need a convention for what fn->virtual_device should be: Wdyt? |
Thanks @mbs-octoml . these are accurate summary of the state. I agree with you that it is not worth to go with c(high order machinary). Like I said I want to mainly clarifying the choices and and suggest documenting the semantics also in the IR data structure (assuming there will be alternative versions of device planner or consumers of the IR who will need to make use of such information, they will the read comments on the data structure for semantics). From what I can see the current set of discussions are great and already comprehensive and I trust @electriclilies and you to make the call here. |
Great, thanks. Ok my suggestion then is to stick with b) but beef up the comment Line 179 in 1ac01b4
to capture some of the 'hidden semantics' buried in the device planner commentary: tvm/src/relay/transforms/device_planner.cc Line 241 in 1ac01b4
|
Thanks @electriclilies @mbs-octoml ! |
080c8b3
to
9fa7f83
Compare
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.
I think that is indeed it, thanks. When we play the equivalent game for the arguments it may be that some FunctionOnDevice calls are unnecessary since the various WithFields will have already propagated the virtual_device_ on both params and function, but that will work itself out in the wash easily enough.
@mbs-octoml OK, so I ended up changing the parser to parse the virtual device in for the tests. I'm not married to the syntax-- right now I'm just putting the virtual device right after the return type of the function, separated by a comma. I'm thinking this will generalize well to parameter types. So for now it''s like this:
One thing I wasn't sure about was the approach for keywords in the parser-- I couldn't find where the keywords are checked, so I just am letting virtual_device be any identifier for now. @jroesch any insight on this? |
src/parser/parser.cc
Outdated
@@ -1130,16 +1146,25 @@ class Parser { | |||
ret_type = ParseType(); | |||
} | |||
|
|||
ObjectRef virtual_device; | |||
if (WhenMatch(TokenType::kComma)) { | |||
virtual_device = ParseVirtualDevice(); |
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.
Ideal would be if we could parse the attributes as usual, then 'promote' the "virtual_device" attribute to the first class field. Since meta references are handled by ParseAttributeValue I think that would work out.
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.
OK, can do that
I realized that the representation of the program after device planning needs some more thought, and this influences how the device planned program is represented in relay script. Pausing work on this PR till I have done a more detailed design review. |
d2ead2d
to
3e5836b
Compare
I ended up just lifting the result virtual device out of the attributes, will discuss dealing with the text representation in another PR. |
3a462b2
to
c8a920e
Compare
Thanks @masahi! |
…ual_device_ field (apache#9848) * VStore function result virtual devices in virtual_device_ field * Address Mark's 'mega nit' * Promote function result virtual device to first class * Add kVirtualDevice * move kVirtualDevice * Fix annotation test * Progress on parsing & printing * Fix printing of virtual device attribute * flake
In this PR, I store the virtual device for a function result in the virtual_device_ field instead of in the function attributes. I am splitting this out from #9666 to isolate issues, and there will be a follow up PR moving the virtual devices of the function parameters into the virtual_device_ fields of the parameters themselves.