-
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] Use cblas for dense and batch_matmul when "cblas" is in the target libraries #3787
Conversation
@hlu1, since you have a similar version. Could you take a look? |
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 perf wise, it would make more sense to use AutoTVM to choose whether to use cblas
based on tuning results, instead of hardcoding it.
cc @icemelon9, @kevinthesun
topi/python/topi/nn/batch_matmul.py
Outdated
x : tvm.Tensor | ||
3-D with shape [batch, M, K] | ||
|
||
y : tvm.TEnsor |
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.
type: y : tvm.Tensor
Please fix the same typo on line 32 as well
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.
Fixed
topi/python/topi/x86/batch_matmul.py
Outdated
x : tvm.Tensor | ||
3-D with shape [batch, M, K] | ||
|
||
y : tvm.TEnsor |
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.
same typo
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.
Fixed
Generally I would agree, but in this case, batch_matmul doesn't have any tuning schedules defined right now. Also, this code path is only hit when you explicitly specify "cblas" in the libs. If the user sets that as their library, I feel like it's expected to use BLAS. I think we can do tuning of BLAS vs TVM schedules in another PR. Also, dense already has this support for cublas and rocblas. It's just missing for cblas. |
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.
Looks good. I can send a follow-up PR to fix the autotuning if you don't mind. I basically have it working.
Please fix CI issue. |
…rget libraries (apache#3787) * Support cblas library in dense * start to add support for generic batch_matmul compute * Add x86 override for batch_matmul * Fix linting * reset file * Fix typos * dummy change to re-trigger CI
…rget libraries (apache#3787) * Support cblas library in dense * start to add support for generic batch_matmul compute * Add x86 override for batch_matmul * Fix linting * reset file * Fix typos * dummy change to re-trigger CI
…rget libraries (apache#3787) * Support cblas library in dense * start to add support for generic batch_matmul compute * Add x86 override for batch_matmul * Fix linting * reset file * Fix typos * dummy change to re-trigger CI
Use cblas for dense and batch_matmul when "cblas" is in the target libraries. This is already supported for cublas and rocblas, it's just not plugged in for cblas. Also added overridden batch_matmul compute for x86.
@ajtulloch @yinghai would you be able to review?