-
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
add use_mkldnn attribute to ops in dygraph #25773
add use_mkldnn attribute to ops in dygraph #25773
Conversation
Thanks for your contribution! |
python/paddle/fluid/layers/nn.py
Outdated
@@ -11322,7 +11322,7 @@ def scale(x, scale=1.0, bias=0.0, bias_after_scale=True, act=None, name=None): | |||
return helper.append_activation(out) | |||
|
|||
|
|||
def elementwise_add(x, y, axis=-1, act=None, name=None): | |||
def elementwise_add(x, y, axis=-1, act=None, name=None, use_mkldnn=False): |
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.
The fate of this line shall be decided after we resolve the issue discussed here: #25713 (comment)
@luotao1 As suggested here (#25713 (comment)), I've changed a way oneDNN is called from |
@luotao1 Could you send me logs of MAC_PR_CI_python35 or rerun if it's a random fail? |
It seems not random fail, but I rerun it again for you. |
@grygielski you can download this log by https://paddle-docker-tar.cdn.bcebos.com/buildLog/386139.log |
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
|
python/paddle/fluid/dygraph/nn.py
Outdated
@@ -810,6 +831,7 @@ def __init__(self, | |||
pool_padding=0, | |||
global_pooling=False, | |||
use_cudnn=True, | |||
use_mkldnn=True, |
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.
Since we use FLAGS_use_mkldnn
to global control, do you need to add use_mkldnn
attribute in each API?
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.
As far as I understood our agreement, we didn't want to add use_mkldnn attribute in standard op API, thus we have decided to use Flags here. However for dygraph ops we wanted to be able to control every op whether it should be executed with oneDNN or not. Similarly these ops have use_cuda attribute. Am I wrong?
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.
@phlrain What's your opinion about it?
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.
However for dygraph ops we wanted to be able to control every op whether it should be executed with oneDNN or not
@grygielski Discussed with @phlrain, we think FLAGS_use_mkldnn
is enough for dygraph, we don't need to control every op whether it should be executed with oneDNN or not
3584eb0
to
0da66fd
Compare
480ed1d
to
086604c
Compare
@luotao1 I've removed |
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
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
PR types
New features
PR changes
APIs
Describe
Adds use_mkldnn argument to ops, similarly to how use_cudnn is used. In Dygraph mode, setting global FLAGS_use_mkldnn=true enables mkldnn for all supported operators. If it is desired to have non-mkldnn op when FLAGS_use_mkldnn=true, it can be done by passing use_mkldnn=False to Op as argument.