-
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
[AutoSchedule] Sparse dense tuning support with custom sketch rule #7313
Conversation
Thanks @jcf94 for the PR! |
python/tvm/auto_scheduler/measure.py
Outdated
density *= i | ||
density /= (K * N) | ||
density = density.value | ||
sparse_prefix = "%s_%d_%d_%d_%d_%d_%.2f_" % ( |
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.
We could run into the case that two matrices have the same sparse_prefix
, but different non-zero structure. Will this cause issues? What if one of the matrices has one nonzero per row and the other has one dense row (while maintaining the same sparsity)?
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.
Though in my test a schedule seems to have similar performance with different random sparse data, I think that may still be a potential problem. Unfortunately, I have not figured out any better solution.
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 could hash the indptr
and indices
arrays as these determine the structure. Alternatively you could hash the number of nonzeros per row.
It would be interesting to study if tuning performs the same independent of structure (but for the same sparsity).
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.
minor style comments
According to our offline discussion,
|
@comaniac @merrymercy Comments all addressed: |
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 just want to echo my earlier concerns about special casing sparse inputs instead of having a generic mechanism for detecting special task inputs.
python/tvm/auto_scheduler/measure.py
Outdated
tensor_input_map[arg] = arg.op.name | ||
|
||
# Case 1: Check sparse op | ||
sparse_input_map = topi.nn.sparse.try_get_sparse_input(args) |
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 I asked this before, but can we have a more general mechanism than checking only for sparse. There are other use cases that require specific input (sorting, scatter).
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, I've also had some discussions in our weekly sync while didn't figure out any better solutions.
There're several reasons:
- Different ops have different requirements over specific inputs;
- While the problems is in a subgraph generated in Relay Integration, the placeholder are all the same, we can not differentiate them from tag, name or any other way, even the order of inputs are not guaranteed.
Current approach is to merge all specific inputs checking to this function, at least they have a same entry here. For the other ops, you have to add their own check functions below.
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.
By the way, my colleague is going to add Ansor support for sparse_conv2d. We'll add extra check to this entry first, and see if there's any better way to merge them.
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.
Could we associate the lookup mechanism with @register_workload
? It would at least be extensible then.
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.
Could we associate the lookup mechanism with
@register_workload
? It would at least be extensible then.
Thanks! This is a pretty good idea, I'll have a try.
@merrymercy @comaniac @tkonolige Thanks! Comments has all been addressed. |
…pache#7313) * Add sparse dense tuning tutorial * Add sparse input fusion * Update the dag to support output fusion * Update * Add task input to search_task * Update * Add search_inputs to measure * Lint fix * Lint fix * Update * Update * Update * Update * Add file save load support * Update * Update * Update * Remove add_task_inputs API * Update * Update * Update * Lint fix * Lint fix * Lint fix * Lint fix * Update * Add example ci_log * Update * retrigger ci * Update * Update * Update * Lint fix * Lint fix * Lint fix
…pache#7313) * Add sparse dense tuning tutorial * Add sparse input fusion * Update the dag to support output fusion * Update * Add task input to search_task * Update * Add search_inputs to measure * Lint fix * Lint fix * Update * Update * Update * Update * Add file save load support * Update * Update * Update * Remove add_task_inputs API * Update * Update * Update * Lint fix * Lint fix * Lint fix * Lint fix * Update * Add example ci_log * Update * retrigger ci * Update * Update * Update * Lint fix * Lint fix * Lint fix
No description provided.