Skip to content
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

[Strategy][ARM CPU] Remove contrib spatial pack schedule of depthwise convolution #5148

Merged
merged 2 commits into from
Mar 26, 2020

Conversation

FrozenGene
Copy link
Member

The spatial pack schedule of depthwise convolution is under the tag of contrib before introducing op strategy. However, as this discuss: https://discuss.tvm.ai/t/autotuner-incorrect-result-after-tuning-mobilenetv2-on-arm-cpu/6088, we maybe will meet incorrect result use this schedule. I ever met this problem on some hardware platform too. However, it is not 100% could produce and happened rarely. Correct result should be firstly, so let us make our previous depthwise schedule be the default, not the contrib spatial pack schedule.

@icemelon9 Could you help to review the code?

@FrozenGene FrozenGene requested a review from icemelon March 25, 2020 16:15
@FrozenGene FrozenGene changed the title [Strategy][ARM CPU] Low the plevel of contrib spatial pack of depthwise conv [Strategy][ARM CPU] Lower the plevel of contrib spatial pack schedule of depthwise convolution Mar 25, 2020
@FrozenGene FrozenGene changed the title [Strategy][ARM CPU] Lower the plevel of contrib spatial pack schedule of depthwise convolution [Strategy][ARM CPU] Remove contrib spatial pack schedule of depthwise convolution Mar 25, 2020
@FrozenGene
Copy link
Member Author

After thinking it a bit more, lower the plevel could only change the default schedule. However, when the users tune it by themselves, they will still introduce this contrib schedule into their tuning task. Before we could solve this problem, I would like to comment it out (but keep the code there). What about your advices? @comaniac @icemelon9

@comaniac
Copy link
Contributor

After thinking it a bit more, lower the plevel could only change the default schedule. However, when the users tune it by themselves, they will still introduce this contrib schedule into their tuning task. Before we could solve this problem, I would like to comment it out (but keep the code there). What about your advices? @comaniac @icemelon9

You are right. That's the intention of the plevel in strategy. If this schedule has a potential correctness issue, we could have the following 3 solutions:

  1. Suggest users enabling correctness checking when tuning (i.e., let AutoTVM check correctness for each schedule config), but this will also check the other task that does not have this issue, making the tuning time even longer.

  2. Add an optional when registering an AutoTVM task to enforce correctness checking, but this requires a change to not only the registration but AutoTVM mechanism.

  3. Comment out the schedule and let it back after the issue has been resolved. The problem is that we may never let it back if no one is dedicated to fix it...

@icemelon9 do you have any suggestions?

@icemelon
Copy link
Member

Yes, let's comment out this implementation temporarily. Could you add a Todo with pointer to the issue in the discussion to both arm cpu strategy and the topi file?

@FrozenGene
Copy link
Member Author

Yes, let's comment out this implementation temporarily. Could you add a Todo with pointer to the issue in the discussion to both arm cpu strategy and the topi file?

Sure. I have added comments as your advice. @comaniac @icemelon9 please have a look again

@FrozenGene FrozenGene merged commit 314f31b into apache:master Mar 26, 2020
@FrozenGene
Copy link
Member Author

Thanks @icemelon9 @comaniac for reviewing and giving advices.

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Apr 16, 2020
… convolution (apache#5148)

* [Strategy][ARM CPU] Low the plevel of contrib spatial pack of depthwise convolution

* address comments
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Apr 17, 2020
… convolution (apache#5148)

* [Strategy][ARM CPU] Low the plevel of contrib spatial pack of depthwise convolution

* address comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants