Skip to content
This repository was archived by the owner on Nov 17, 2023. It is now read-only.

[v1.x][BUGFIX] Impose a plain format for concat's output when oneDNN would use padding - #19735

Merged
szha merged 7 commits into
apache:v1.xfrom
PawelGlomski-Intel:concatFix
Jan 26, 2021
Merged

[v1.x][BUGFIX] Impose a plain format for concat's output when oneDNN would use padding#19735
szha merged 7 commits into
apache:v1.xfrom
PawelGlomski-Intel:concatFix

Conversation

@PawelGlomski-Intel

@PawelGlomski-Intel PawelGlomski-Intel commented Jan 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Right now, oneDNN may choose a blocked format for the output of concat, in cases that may require additional padding (thus more memory). In (de)convolution we make sure to only select a primitive_desc which has the expected (from tensor size) memory size requirements, so the same has to be done in this case.

Unlike convolution's, concat's primitive_desc does not support iteration over multiple implementations, so here I manually impose a plain format when oneDNN would choose a blocked one with padding.

Resolves #19586

Checklist

Essentials

  • PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented

Changes

  • Impose a plain format for concat's output when mkldnn uses padding
  • Fix and add missing comments related to memory planning and padded formats
  • Add/Change tests that cover this case

@mxnet-bot

mxnet-bot commented Jan 8, 2021

Copy link
Copy Markdown

Hey @PawelGlomski-Intel , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [website, centos-gpu, sanity, windows-gpu, miscellaneous, clang, windows-cpu, centos-cpu, unix-cpu, unix-gpu, edge]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@lanking520 lanking520 added the pr-work-in-progress PR is still work in progress label Jan 8, 2021
@PawelGlomski-Intel PawelGlomski-Intel changed the title [v1.x][BUGFIX][WIP] Impose a plain format for concat's output when oneDNN would use padding [v1.x][BUGFIX] Impose a plain format for concat's output when oneDNN would use padding Jan 12, 2021
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Jan 12, 2021
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-awaiting-review PR is waiting for code review labels Jan 14, 2021
@PawelGlomski-Intel

PawelGlomski-Intel commented Jan 14, 2021

Copy link
Copy Markdown
Contributor Author

Sorry for incidental review requests, I pushed a wrong branch

@lanking520 lanking520 added pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress labels Jan 14, 2021
@lanking520 lanking520 added pr-awaiting-review PR is waiting for code review pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review labels Jan 14, 2021
@PawelGlomski-Intel

PawelGlomski-Intel commented Jan 14, 2021

Copy link
Copy Markdown
Contributor Author

@mxnet-bot run ci [unix-gpu]

@mxnet-bot

mxnet-bot commented Jan 14, 2021

Copy link
Copy Markdown

Jenkins CI successfully triggered : [unix-gpu]

@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Jan 14, 2021
@PawelGlomski-Intel

PawelGlomski-Intel commented Jan 18, 2021

Copy link
Copy Markdown
Contributor Author

@mxnet-bot run ci [unix-gpu]

@mxnet-bot

mxnet-bot commented Jan 18, 2021

Copy link
Copy Markdown

Jenkins CI successfully triggered : [unix-gpu]

@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Jan 18, 2021
@PawelGlomski-Intel

PawelGlomski-Intel commented Jan 25, 2021

Copy link
Copy Markdown
Contributor Author

@PatricZhao, @szha could you please review this?

@szha
szha merged commit 49b26b6 into apache:v1.x Jan 26, 2021
@szha

szha commented Jan 26, 2021

Copy link
Copy Markdown
Member

Thanks for the fix!

@PawelGlomski-Intel PawelGlomski-Intel mentioned this pull request Apr 6, 2021
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@aaronmarkham aaronmarkham Awaiting requested review from aaronmarkham
@anirudh2290 anirudh2290 Awaiting requested review from anirudh2290
@eric-haibin-lin eric-haibin-lin Awaiting requested review from eric-haibin-lin
@marcoabreu marcoabreu Awaiting requested review from marcoabreu
@szha szha Awaiting requested review from szha
@thirdwing thirdwing Awaiting requested review from thirdwing

Assignees

No one assigned

Labels

pr-awaiting-review PR is waiting for code review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@PawelGlomski-Intel @mxnet-bot @szha @lanking520