-
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
[Test] enable NHWC of relay.testing.mobilenet
#3886
Conversation
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.
LGTM
Should we also consider adding a unit test that would check that |
Hi @tmoreau89 , thank you for the quick review!
Yes, adding a test is good point, I have also evaluated before open this PR. To me, testing |
Good points @jackwish ! It's weird that compiling NHWC is too slow indeed; perhaps it's worth looking into it. Can we at least have a test for |
Marking it is a good idea rather than leaving it there :) I will update the PR. I think the building performance is related with the OP schedules - there is not dedicated NHWC schedules for x86 target (it is pretty fast when we build targeting arm internally). I guess this issue can be fixed when your NHWC schedule (#3858) got merged. :) |
In this way, we can play around NHWC inside TVM regardless of the frontends.
Exactly! I think that if this PR gets merged first, I can just enable the test once performance is reasonable. |
Thank you @tmoreau89 @vinx13 @zhiics @icemelon9 , would you please also take a look at this if it can merge now? Seems the CI was canceled, can we trigger it by hand? |
Looks great, thanks for adding the test. Regarding the CI error I haven't encountered this bug before... Did re-triggering with a minimal commit work around it? |
As for CI I cannot log in to retrigger as well, we can probably just submit an empty commit to run it again. git commit --allow-empty -m "retrigger ci" |
NVM, please just ignore my comment. I just saw you updated. |
Yes, thank you @zhiics |
* [Relay] enable NHWC of `relay.testing.mobilenet` In this way, we can play around NHWC inside TVM regardless of the frontends. * [Test] test for NHWC of relay.testing.mobilenet
* [Relay] enable NHWC of `relay.testing.mobilenet` In this way, we can play around NHWC inside TVM regardless of the frontends. * [Test] test for NHWC of relay.testing.mobilenet
* [Relay] enable NHWC of `relay.testing.mobilenet` In this way, we can play around NHWC inside TVM regardless of the frontends. * [Test] test for NHWC of relay.testing.mobilenet
* [Relay] enable NHWC of `relay.testing.mobilenet` In this way, we can play around NHWC inside TVM regardless of the frontends. * [Test] test for NHWC of relay.testing.mobilenet
This is part of #3859 which enables NHWC for conv2d on ARM devices. Putting it here will be clearer. Please help to review @tqchen @eqy .
EDIT: the force-push fixed lint.