-
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
[TOPI] Prevent scheduling the same operation more than once #1556
Conversation
Thanks @masahi , this is merged! |
@@ -39,10 +39,11 @@ def decl_spatial_pack(cfg, data, kernel, strides, padding, layout, out_dtype): | |||
def schedule_conv2d_nchw_arm_cpu(cfg, outs): | |||
"""TOPI schedule callback""" | |||
s = tvm.create_schedule([x.op for x in outs]) | |||
scheduled_ops = [] |
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.
You should handle this logic in the utility function traverse_inline
.
Currently the traverse inline logic takes much redundant code in TOPI. I recommend that developers should switch to use this utility function.
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.
Yeah it was pain to add all those scheduled_ops stuff to all backends.
But it is not a bug, right?
I was not in a mood to refactor all traverse logic to use traverse_inline.
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.
When I looked at your traverse_inline function, I thought I need to pass around scheduled_ops to traverse_inline function. I didn't want to make this change, so I chose a more straightforward approach.
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.
Here is my local version. Possibly you can update it for arm_cpu only in your NNVM fusion PR.
def traverse_inline(s, final_op, callback):
"""Traverse computation graph and do auto inline
Parameters
----------
s: schedule
The schedule
final_op: Operation
The final output operator.
callback: callable
The callback function on each op
"""
visited = set()
def _traverse(op):
if op in visited:
return
visited.add(op)
if tag.is_injective(op.tag):
if op not in s.outputs:
s[op].compute_inline()
for tensor in op.input_tensors:
if tensor.op.input_tensors:
traverse_inline(s, tensor.op, callback)
callback(op)
_traverse(final_op)
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.
It should be
for tensor in op.input_tensors:
if tensor.op.input_tensors:
_traverse(tensor.op)
no?
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 updated #1548 according to your comment.
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.
Aha, yes..
Split from #1548.
Future updates to NNVM operator fusion may introduce cases where the same op can be reached from more than one route during output-to-input traversal. This PR makes sure an operation is scheduled only once.
This PR will be followed by #1548, where a relevant test case is included.