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.

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

Added softplus FP32 FWD OneDNN kernel #36382

Merged
merged 8 commits into from
Oct 18, 2021
Merged

Conversation

jakpiase
Copy link
Contributor

PR types

New features

PR changes

OPs

Describe

Added softplus FP32 FWD OneDNN kernel. It improves ppyolov2_r50vd_365e_coco model by 14%

@paddle-bot-old
Copy link

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@jakpiase jakpiase added the Intel label Oct 12, 2021
@jakpiase jakpiase requested a review from jczaja October 12, 2021 23:14
@jakpiase
Copy link
Contributor Author

@piotrekobiIntel please review this PR

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Apart from the import error, this looks great :)

jczaja
jczaja previously approved these changes Oct 13, 2021
Copy link
Contributor

@jczaja jczaja left a comment

Choose a reason for hiding this comment

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

LGTM

@jakpiase jakpiase requested a review from jczaja October 13, 2021 18:21
Copy link
Contributor

@jczaja jczaja left a comment

Choose a reason for hiding this comment

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

LGTM

@jakpiase
Copy link
Contributor Author

@piotrekobiIntel Could you please continue your review?

Comment on lines +37 to +49
dnnl::post_ops post_ops;
post_ops.append_eltwise(1.0f, dnnl::algorithm::eltwise_soft_relu, 0.0f,
0.0f);
if (beta != 1.0f) {
post_ops.append_eltwise(1.0f, dnnl::algorithm::eltwise_linear,
1.0f / beta, 0.0f);
}

dnnl::primitive_attr attrs;
attrs.set_post_ops(post_ops);

this->AcquireForwardPrimitiveDescriptor(attrs, dnnl::algorithm::binary_mul,
x_md, beta_md, x_md);
Copy link

Choose a reason for hiding this comment

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

Something like this would allow to skip the multiplication by 1 if beta = 1, this exact code probably won't work but I hope you understand what I mean. I'm not sure if it's worth putting in the extra work though (it depends how often beta is equal to 1 in practice).

Suggested change
dnnl::post_ops post_ops;
post_ops.append_eltwise(1.0f, dnnl::algorithm::eltwise_soft_relu, 0.0f,
0.0f);
if (beta != 1.0f) {
post_ops.append_eltwise(1.0f, dnnl::algorithm::eltwise_linear,
1.0f / beta, 0.0f);
}
dnnl::primitive_attr attrs;
attrs.set_post_ops(post_ops);
this->AcquireForwardPrimitiveDescriptor(attrs, dnnl::algorithm::binary_mul,
x_md, beta_md, x_md);
if (beta == 1.0f)
{
this->AcquireForwardPrimitiveDescriptor(attrs, dnnl::algorithm::eltwise_soft_relu, x_md, x_md);
}
else
{
dnnl::post_ops post_ops;
post_ops.append_eltwise(1.0f, dnnl::algorithm::eltwise_soft_relu, 0.0f,
0.0f);
post_ops.append_eltwise(1.0f, dnnl::algorithm::eltwise_linear,
1.0f / beta, 0.0f);
dnnl::primitive_attr attrs;
attrs.set_post_ops(post_ops);
this->AcquireForwardPrimitiveDescriptor(attrs, dnnl::algorithm::binary_mul,
x_md, beta_md, x_md);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done like that in the previous commits of this PR, but I have agreed with Jacek, that the overall change in performance was meaningless, and this way the code is unified and much more clear. Moreover, this operator will be fused with tanh activation(for ppyolov2_r50vd_365e model), so in that case binary operation must be done, because eltwise primitive does not support fusing with another eltwise primitive. But you've definitely got a point that execution time would be faster if there would be just soft_relu without binary_mul at the beginning

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Great, LGTM then :)

@jczaja jczaja merged commit bdac9ff into PaddlePaddle:develop Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants