-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[new-exec] fit mkldnn #41058
[new-exec] fit mkldnn #41058
Conversation
你的PR提交成功,感谢你对开源项目的贡献! |
// NOTE(zhiqiu): hot fix, follow the same logic in DataCopy() in fetch_op.cc | ||
if (in_layout == framework::DataLayout::kMKLDNN && | ||
var_name == framework::GradVarName("Filter") && is_fetch_v2) { | ||
out_layout = framework::DataLayout::kNCHW; |
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.
Why out_layout is set to kNCHW ?
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.
I don't really understand the tricky, just follow DataCopy() in fetch_op.cc
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.
Ok. Thanks I understand now.
VLOG(4) << in_layout << "->" << out_layout << " " << in_tensor.layout(); | ||
if (!in_tensor.IsInitialized() && in_layout == DataLayout::kMKLDNN && | ||
out_layout == DataLayout::kNHWC) { |
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.
Recently DataLayout was extended with 5D NHWC layout e.g. kNDHWC for which also MatchShapeToLayout() should be called
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.
Ok, I think this can be refined in the future, here just follow the same logic in executor.cc
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.
ok
@zhiqiu We just started our own tests on this PR just to see functional and performance impact of changes . We will come back to you when we have results (this week) so please wait with merging |
@jczaja Thanks. Since paddle is approaching release-2.3, and there are some works depending on this PR, can we merge it first? If any problems are found, we can fix them in the next PR. |
@zhiqiu Yes. we just finished light testing and this PR passed our C++ workloads . So should be fine. It can be merged. |
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
Thanks. |
PR types
New features
PR changes
Others
Describe
fit mkldnn for new executor