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.

Sign up for GitHub

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

Jump to bottom

[BYOC] Fix DNNL Conv2D in JSON runtime #9043

Merged
merged 3 commits into from
Sep 23, 2021
Merged

[BYOC] Fix DNNL Conv2D in JSON runtime #9043

merged 3 commits into from
Sep 23, 2021

Conversation

sunwayforever
Copy link
Contributor

@sunwayforever sunwayforever commented Sep 18, 2021 •
edited
Loading

There are some typoes when handling conv2d shapes in dnnl. the original test
case doesn't cover conditions like conv2d with different input height/width, or
with different top/bottom padding.

@comaniac
Copy link
Contributor

Thanks for the fix. Could you please finish the PR description and add a test case?

@comaniac comaniac changed the title dnnl memory dim is wrong for conv2d [BYOC] dnnl memory dim is wrong for conv2d Sep 18, 2021
@comaniac comaniac changed the title [BYOC] dnnl memory dim is wrong for conv2d [BYOC] Fix DNNL Conv2D in JSON runtime Sep 18, 2021
@sunwayforever
Copy link
Contributor Author

sunwayforever commented Sep 19, 2021 •
edited
Loading

Thanks for the fix. Could you please finish the PR description and add a test case?

@comaniac thanks for your reply. is there any guideline for adding a test case? should I just add some more tests in test/python/relay/test_json_runtime.py?

@comaniac
Copy link
Contributor

Thanks for the fix. Could you please finish the PR description and add a test case?

@comaniac thanks for your reply. is there any guideline for adding a test case? should I just add some more tests in test/python/relay/test_json_runtime.py?

Yes. Basically you just need to add a new test or improve an existing test that will fail without this PR, so that we could make sure the fix covered by this PR won't be broken in the future.

comaniac
comaniac approved these changes Sep 22, 2021
View reviewed changes
Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The CI error seems not relevant so you probably need to just re-trigger it.

@sunwayforever
Copy link
Contributor Author

LGTM. The CI error seems not relevant so you probably need to just re-trigger it.

how to re-trigger CI? git commit --amend then force-push the commit?

@comaniac
Copy link
Contributor

Just push an empty commit with --allow-empty.

@masahi masahi merged commit 0d86621 into apache:main Sep 23, 2021
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
@sunwayforever @ylc
[BYOC] Fix DNNL Conv2D in JSON runtime (apache#9043)
3183612 
* dnnl memory dim is wrong for conv2d

* change test case for dnnl conv2d

* trigger CI

Co-authored-by: sunway <sunwayforever@hotmail.com>
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@sunwayforever @ylc
[BYOC] Fix DNNL Conv2D in JSON runtime (apache#9043)
8e8b038 
* dnnl memory dim is wrong for conv2d

* change test case for dnnl conv2d

* trigger CI

Co-authored-by: sunway <sunwayforever@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@comaniac comaniac comaniac approved these changes

@areusch areusch Awaiting requested review from areusch

@junrushao junrushao Awaiting requested review from junrushao

@kazum kazum Awaiting requested review from kazum

@liangfu liangfu Awaiting requested review from liangfu

@masahi masahi Awaiting requested review from masahi

@tmoreau89 tmoreau89 Awaiting requested review from tmoreau89

@tqchen tqchen Awaiting requested review from tqchen

@vinx13 vinx13 Awaiting requested review from vinx13

@ZihengJiang ZihengJiang Awaiting requested review from ZihengJiang

@jroesch jroesch Awaiting requested review from jroesch

@merrymercy merrymercy Awaiting requested review from merrymercy

@yzhliu yzhliu Awaiting requested review from yzhliu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sunwayforever @comaniac @masahi