-
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
[Relay] Prepare for switching VM to LowerTEPass. #9550
Conversation
7d47a03
to
50898e1
Compare
This is a grab bag of fallout changes from switching the VM to use LoweTEPass which can be ealy split out of the main apache#9483 PR. - AnnotateSpans can be used from C++ (though, unfortunately, it didn't help me with debugging since spans are universally dropped in most passes). - Can get a human readable dump of the VM's PackedFunc names and indexes for debugging. - If TVM_LOG_DEBUG defined then include types and ids of GlobalVars. I had a lot of difficulty tracking down where duplicate GlobalVars for the same name_hint were getting created and propagated. - GetCallLoweredProps follows same API as GetDeviceCopy and GetOnDevice where will return 'null' properties if call/expr is not of call_lowered form. Mildly more convenient, though switching all the above to ICHECK and push 'if (op == the relevant op)' into all use sites would also be just fine. - Misc VLOG improvements made while tracking down issues in apache#9483.
50898e1
to
700669b
Compare
@@ -144,6 +144,13 @@ class Executable : public ModuleNode { | |||
*/ | |||
std::string GetVirtualDevices() const; | |||
|
|||
/*! |
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.
Can you clarify what exactly the return value is here given its just a std::string
.
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.
My I please do that in the sequel so we can merge and I take 9483 out of draft? These are just human readable pretty printed descriptions so I can decipher the instructions. Thx.
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.
Please could someone take a look at the test coverage I've highlighted, but overall this makes sense to me as an intermediary state of being and I think the comment can be addressed in the already available follow up PR 😸
@@ -95,20 +96,29 @@ RELAY_REGISTER_OP("call_lowered") | |||
.set_attr<FInferCorrectLayout>("FInferCorrectLayout", ElemwiseArbitraryLayout); | |||
|
|||
CallLoweredProps GetCallLoweredProps(const CallNode* call_node) { |
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.
Given this is such a fundamental function it seems like we should have test cases for all the different cases it throws an exception and what we expect as a return value. Though I equally understand this wasn't authored in this PR.
@mbs-octoml / @electriclilies can you take a look at this?
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.
Thanks @Mousius, agree and sorry I missed that in the orig PR. Can you take that @electriclilies ? Thanks!
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.
Yup!
Landing this to move onto #9483, thanks @mbs-octoml ! |
This is a grab bag of fallout changes from switching the VM to use LoweTEPass which can be easily split out of the main apache#9483 PR. - AnnotateSpans can be used from C++ (though, unfortunately, it didn't help me with debugging since spans are universally dropped in most passes). - Can get a human readable dump of the VM's PackedFunc names and indexes for debugging. - If TVM_LOG_DEBUG defined then include types and ids of GlobalVars. I had a lot of difficulty tracking down where duplicate GlobalVars for the same name_hint were getting created and propagated. - GetCallLoweredProps follows same API as GetDeviceCopy and GetOnDevice where will return 'null' properties if call/expr is not of call_lowered form. Mildly more convenient, though switching all the above to ICHECK and push 'if (op == the relevant op)' into all use sites would also be just fine. - Misc VLOG improvements made while tracking down issues in apache#9483.
This is a grab bag of fallout changes from switching the VM to use LoweTEPass which can be easily split out of the main apache#9483 PR. - AnnotateSpans can be used from C++ (though, unfortunately, it didn't help me with debugging since spans are universally dropped in most passes). - Can get a human readable dump of the VM's PackedFunc names and indexes for debugging. - If TVM_LOG_DEBUG defined then include types and ids of GlobalVars. I had a lot of difficulty tracking down where duplicate GlobalVars for the same name_hint were getting created and propagated. - GetCallLoweredProps follows same API as GetDeviceCopy and GetOnDevice where will return 'null' properties if call/expr is not of call_lowered form. Mildly more convenient, though switching all the above to ICHECK and push 'if (op == the relevant op)' into all use sites would also be just fine. - Misc VLOG improvements made while tracking down issues in apache#9483.
This is a grab bag of fallout changes from switching the VM to use LoweTEPass which can be easily split out of the main apache#9483 PR. - AnnotateSpans can be used from C++ (though, unfortunately, it didn't help me with debugging since spans are universally dropped in most passes). - Can get a human readable dump of the VM's PackedFunc names and indexes for debugging. - If TVM_LOG_DEBUG defined then include types and ids of GlobalVars. I had a lot of difficulty tracking down where duplicate GlobalVars for the same name_hint were getting created and propagated. - GetCallLoweredProps follows same API as GetDeviceCopy and GetOnDevice where will return 'null' properties if call/expr is not of call_lowered form. Mildly more convenient, though switching all the above to ICHECK and push 'if (op == the relevant op)' into all use sites would also be just fine. - Misc VLOG improvements made while tracking down issues in apache#9483.
This is a grab bag of fallout changes from switching the VM to use LoweTEPass which can be easily split out of the main apache#9483 PR. - AnnotateSpans can be used from C++ (though, unfortunately, it didn't help me with debugging since spans are universally dropped in most passes). - Can get a human readable dump of the VM's PackedFunc names and indexes for debugging. - If TVM_LOG_DEBUG defined then include types and ids of GlobalVars. I had a lot of difficulty tracking down where duplicate GlobalVars for the same name_hint were getting created and propagated. - GetCallLoweredProps follows same API as GetDeviceCopy and GetOnDevice where will return 'null' properties if call/expr is not of call_lowered form. Mildly more convenient, though switching all the above to ICHECK and push 'if (op == the relevant op)' into all use sites would also be just fine. - Misc VLOG improvements made while tracking down issues in apache#9483.
This is a grab bag of fallout changes from switching the VM to use LoweTEPass which can be easily split out of the main apache#9483 PR. - AnnotateSpans can be used from C++ (though, unfortunately, it didn't help me with debugging since spans are universally dropped in most passes). - Can get a human readable dump of the VM's PackedFunc names and indexes for debugging. - If TVM_LOG_DEBUG defined then include types and ids of GlobalVars. I had a lot of difficulty tracking down where duplicate GlobalVars for the same name_hint were getting created and propagated. - GetCallLoweredProps follows same API as GetDeviceCopy and GetOnDevice where will return 'null' properties if call/expr is not of call_lowered form. Mildly more convenient, though switching all the above to ICHECK and push 'if (op == the relevant op)' into all use sites would also be just fine. - Misc VLOG improvements made while tracking down issues in apache#9483.
This is a grab bag of fallout changes from switching the VM to use LoweTEPass which can be easily split out of the main apache#9483 PR. - AnnotateSpans can be used from C++ (though, unfortunately, it didn't help me with debugging since spans are universally dropped in most passes). - Can get a human readable dump of the VM's PackedFunc names and indexes for debugging. - If TVM_LOG_DEBUG defined then include types and ids of GlobalVars. I had a lot of difficulty tracking down where duplicate GlobalVars for the same name_hint were getting created and propagated. - GetCallLoweredProps follows same API as GetDeviceCopy and GetOnDevice where will return 'null' properties if call/expr is not of call_lowered form. Mildly more convenient, though switching all the above to ICHECK and push 'if (op == the relevant op)' into all use sites would also be just fine. - Misc VLOG improvements made while tracking down issues in apache#9483.
This is a grab bag of fallout changes from switching the VM to use LoweTEPass which can be easily split out of the main apache#9483 PR. - AnnotateSpans can be used from C++ (though, unfortunately, it didn't help me with debugging since spans are universally dropped in most passes). - Can get a human readable dump of the VM's PackedFunc names and indexes for debugging. - If TVM_LOG_DEBUG defined then include types and ids of GlobalVars. I had a lot of difficulty tracking down where duplicate GlobalVars for the same name_hint were getting created and propagated. - GetCallLoweredProps follows same API as GetDeviceCopy and GetOnDevice where will return 'null' properties if call/expr is not of call_lowered form. Mildly more convenient, though switching all the above to ICHECK and push 'if (op == the relevant op)' into all use sites would also be just fine. - Misc VLOG improvements made while tracking down issues in apache#9483.
This is a grab bag of fallout changes from switching the VM to use LoweTEPass
which can be ealy split out of the main #9483 PR.
me with debugging since spans are universally dropped in most passes).
debugging.
a lot of difficulty tracking down where duplicate GlobalVars for the same
name_hint were getting created and propagated.
where will return 'null' properties if call/expr is not of call_lowered
form. Mildly more convenient, though switching all the above to ICHECK
and push 'if (op == the relevant op)' into all use sites would also be just
fine and I can be talked into that too.
Targets and SEScopes are compared by pointer equality, and the same Target with
and without a host are distinct objects, this was causing unnecessary copies in code which
is already dealing with the explicit host_target or host_se_scope anyway. I've left the hosts
in the legacy_target_map. (The sooner we sort out multi-target compilation and hosts the
better!)