You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/12/10 14:28:58 UTC
[GitHub] [tvm] jacobbohlin opened a new pull request #9706: [microNPU] Fix incorrect comparison in planning functions
jacobbohlin opened a new pull request #9706:
URL: https://github.com/apache/tvm/pull/9706
Bug fix in the `copy_constants()` and `copy_luts() `planning functions. The condition is meant to check whether the `tensor` is in the `planned` set but was incorrectly checking Object equality.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm edited a comment on pull request #9706: [microNPU] Fix incorrect comparison in planning functions
Posted by GitBox <gi...@apache.org>.
manupa-arm edited a comment on pull request #9706:
URL: https://github.com/apache/tvm/pull/9706#issuecomment-991090858
Thanks @jacobbohlin for fixing this!
Should we add a test case too ?
I would suggest adding either a) network test using APIs b) lower_to_tir test to see if expected TIR is produced when using copy constant scheduler or c) use a TE-based test.
For a), use simliar API like this to create a small graph : https://github.com/apache/tvm/blob/aa99699e532ec8c60afd552e951d238b481c4b5d/tests/python/contrib/test_ethosu/test_codegen.py#L162-L192
and then use https://github.com/apache/tvm/blob/aa99699e532ec8c60afd552e951d238b481c4b5d/tests/python/contrib/test_ethosu/test_codegen.py#L389 this for comparison.
cc : @dchauhan-arm
For b) you could use relay snippets such as : https://github.com/apache/tvm/blob/aa99699e532ec8c60afd552e951d238b481c4b5d/tests/python/contrib/test_ethosu/test_legalize.py#L62-L79
and then use a test like this : https://github.com/apache/tvm/blob/aa99699e532ec8c60afd552e951d238b481c4b5d/tests/python/contrib/test_ethosu/test_compiler.py#L25-L44
For c) https://github.com/apache/tvm/blob/aa99699e532ec8c60afd552e951d238b481c4b5d/tests/python/contrib/test_ethosu/test_scheduler.py#L155-L166
I think for the given bug c) is smallest reproducer if provide a diamond graph while a) being a more end to end test.
cc : @lhutton1 @ekalda @NicolaLancellotti
What do others think ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm merged pull request #9706: [microNPU] Fix incorrect comparison in schedulers
Posted by GitBox <gi...@apache.org>.
manupa-arm merged pull request #9706:
URL: https://github.com/apache/tvm/pull/9706
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda commented on pull request #9706: [microNPU] Fix incorrect comparison in planning functions
Posted by GitBox <gi...@apache.org>.
ekalda commented on pull request #9706:
URL: https://github.com/apache/tvm/pull/9706#issuecomment-991110651
Thanks for fixing it @jacobbohlin! Yeah I agree with Manupa that (c) is probably the smallest unit test (though arguably also the most cryptic one), but considering that this bug showed itself only in the case of diamond graphs, it means we don't also have any integration tests for diamond graphs, so it would be good to have some in general, in case the NPU compiler struggles with it in any other stage as well (we might want to add a proper integration test in a different PR though).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on pull request #9706: [microNPU] Fix incorrect comparison in schedulers
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on pull request #9706:
URL: https://github.com/apache/tvm/pull/9706#issuecomment-998983695
Thanks! @ekalda @lhutton1. This is merged now!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jacobbohlin commented on pull request #9706: [microNPU] Fix incorrect comparison in planning functions
Posted by GitBox <gi...@apache.org>.
jacobbohlin commented on pull request #9706:
URL: https://github.com/apache/tvm/pull/9706#issuecomment-991123604
@manupa-arm @ekalda I think c) sounds sensible for this PR, I will add it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on pull request #9706: [microNPU] Fix incorrect comparison in planning functions
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on pull request #9706:
URL: https://github.com/apache/tvm/pull/9706#issuecomment-991090858
Thanks @jacobbohlin for fixing this!
Should we add a test case too ?
I would suggest adding either a) network test using APIs b) lower_to_tir test to see if expected TIR is produced when using copy constant scheduler or c) use a TE-based test.
For a), use simliar API like this to create a small graph : https://github.com/apache/tvm/blob/aa99699e532ec8c60afd552e951d238b481c4b5d/tests/python/contrib/test_ethosu/test_codegen.py#L162-L192
and then use https://github.com/apache/tvm/blob/aa99699e532ec8c60afd552e951d238b481c4b5d/tests/python/contrib/test_ethosu/test_codegen.py#L389 this for comparison.
cc : @dchauhan-arm
For b) you could use relay snippets such as : https://github.com/apache/tvm/blob/aa99699e532ec8c60afd552e951d238b481c4b5d/tests/python/contrib/test_ethosu/test_legalize.py#L62-L79
and then use a test like this : https://github.com/apache/tvm/blob/aa99699e532ec8c60afd552e951d238b481c4b5d/tests/python/contrib/test_ethosu/test_compiler.py#L25-L44
For c) https://github.com/apache/tvm/blob/aa99699e532ec8c60afd552e951d238b481c4b5d/tests/python/contrib/test_ethosu/test_scheduler.py#L155-L166
I think for the given bug c) is smallest reproducer if provide a diamond graph while a) being a more end to end test.
cc : @lhutton1 @ekalda
What do others think ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jacobbohlin commented on pull request #9706: [microNPU] Fix incorrect comparison in planning functions
Posted by GitBox <gi...@apache.org>.
jacobbohlin commented on pull request #9706:
URL: https://github.com/apache/tvm/pull/9706#issuecomment-998211292
@manupa-arm Yes that would be good, I don't know how (or if) I can do that though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on pull request #9706: [microNPU] Fix incorrect comparison in planning functions
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on pull request #9706:
URL: https://github.com/apache/tvm/pull/9706#issuecomment-991124916
@jacobbohlin sounds good!
@ekalda agreed, we should look to add resnet style network test or a small custom diamond network.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jacobbohlin commented on pull request #9706: [microNPU] Fix incorrect comparison in planning functions
Posted by GitBox <gi...@apache.org>.
jacobbohlin commented on pull request #9706:
URL: https://github.com/apache/tvm/pull/9706#issuecomment-991028658
@manupa-arm @NicolaLancellotti @ekalda @lhutton1 @mbaret
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on pull request #9706: [microNPU] Fix incorrect comparison in planning functions
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on pull request #9706:
URL: https://github.com/apache/tvm/pull/9706#issuecomment-998187169
@jacobbohlin Shall we retrigger the CI ? It seems to be stuck for unknown reason.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org