[oneDNN] Further ops refactoring of oneDNN cache access #33515
Conversation
- compilation fix
|
Thanks for your contribution! |
jczaja
requested review from
arlesniak,
arogowie-intel,
jakpiase and
lidanqing-vv
|
|
||
| bool isBwdCached() { | ||
| const std::string key_pd = key_ + "@bwd_pd"; | ||
| bwd_pd_ = std::static_pointer_cast<typename TBackward::primitive_desc>( |
There was a problem hiding this comment.
Hi I saw you name key_fpd, Maube you could name here backward pd key as key_bpd ?
There was a problem hiding this comment.
Normally key_pd is used everywhere it just at that particular moment I needed two keys so I added key_fpd.
| const std::string key_fpd = key_ + "@fwd_pd"; | ||
| fwd_pd_ = std::static_pointer_cast<typename TForward::primitive_desc>( | ||
| dev_ctx_.GetBlob(key_fpd)); | ||
| return true; |
There was a problem hiding this comment.
fwd_pd_ get value by dev_ctx_.GetBlob, then could it be null_ptr? If it is null will it still return true or something like "return bwd_pd_ == nullptr ? false : true" ?
There was a problem hiding this comment.
if bwd_pd_ is valid so then should be fwd_pd_ . But it is a valid point to add some PADDLE_ENFORCE making sure that fwd_pd_ is also valid .
There was a problem hiding this comment.
@lidanqing-intel I added PADDLE_ENFORCE to make sure fwd_pd_ is set when bwd_pd_ is cached. Please sontinue your review
| template <typename Arg, typename... Args> | ||
| void AcquireForwardPrimitiveDescriptorNonBlocking(Arg&& first_arg, | ||
| Args&&... args) { | ||
| // This is used when we can recreate FWD PD in BWD so |
There was a problem hiding this comment.
These two lines comments are describing void AcquireForwardPrimitiveDescriptor(Arg&& first_arg, Args&&... args) ?
There was a problem hiding this comment.
It is still a relevant comment to this function
| void AcquireBackwardPrimitiveDescriptorNonBlocking(Args&&... args) { | ||
| // fwd_pd_ is set during grad by calling | ||
| // AcquireForwardPrimitiveDescriptorNonBlocking | ||
| // AcquireForwardPrimitiveDescriptor |
There was a problem hiding this comment.
Does these comments "// fwd_pd_ is set during grad by calling // AcquireForwardPrimitiveDescriptor" still suit here? Here it looks only bwd_pd_ is created and cached.
There was a problem hiding this comment.
AcquireBackwardPrimitiveDescriptor is creating BWD_PD only that is correct , but when implementing Handler for grad(backward) you also call AcquireForwardPrimitiveDescriptor which will create FWD_PD. So in grad handler we call AcquireForwardPrimitiveDescriptor and AcquireBackwardPrimitiveDescriptor
|
@juncaipeng Hi, There is a problem PR-CI-Windows build. I had it rerunning three times and still it failes due to not related to PR changes errors: 2021-06-12 22:50:51 failed with: Please help |
|
@jczaja Wangzhen said it was Windows machine issue last week and now it's recovered. But me and wanghzen both can not rerun it because CI says: "This commit exceeds rerun limitation." Please change this PR a bit and submit a new commit so CI could restart. |
jczaja
dismissed stale reviews from jakpiase, arlesniak, and lidanqing-vv
via
891032f
|
@juncaipeng This CheckPRTemplate is being running for many hours . Could you please advice how to make it finished? |
jczaja
changed the title
lidanqing-vv
changed the title
lidanqing-vv
changed the title
In this case, it is necessary to modify the describe of the PR, so the CheckPRTemplate will rerun. |
6 participants
PR types
Performance optimization
PR changes
OPs
Describe