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

[WIP] onednn 3.0 integration #50607

Closed
wants to merge 138 commits into from
Closed

Conversation

jczaja
Copy link
Contributor

@jczaja jczaja commented Feb 17, 2023

PR types

PR changes

Describe

@@ -29,11 +29,10 @@ class SoftplusOneDNNHandler
: funcs::OneDNNHandlerNoCachingT<T, dnnl::binary>(dev_ctx.GetEngine(),
dev_ctx.GetPlace()) {
dnnl::post_ops post_ops;
post_ops.append_eltwise(
1.0f, dnnl::algorithm::eltwise_soft_relu, 0.0f, 0.0f);
post_ops.append_eltwise(dnnl::algorithm::eltwise_soft_relu, 0.0f, 0.0f);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a reminder:
In oneDNN v3 API, the formula of soft_relu is changed, so we may need to set the alpha to 1.f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@YangQun1 Thanks, will remember to modify this

Comment on lines +86 to +91
auto zero_points_md = dnnl::memory::desc(
{1}, dnnl::memory::data_type::s32, dnnl::memory::format_tag::x);
auto zero_points_mem =
dnnl::memory(zero_points_md,
dev_ctx.GetEngine(),
phi::funcs::to_void_cast<int32_t>(&quantization_shift));
Copy link
Contributor

Choose a reason for hiding this comment

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

need to check with shift?

Comment on lines +170 to +172
post_operations.append_eltwise(dnnl::algorithm::eltwise_relu, 0.0f, 0.0f);
post_operations.append_eltwise(
activation_scale, dnnl::algorithm::eltwise_relu, 0.0f, 0.0f);
dnnl::algorithm::eltwise_linear, activation_scale, 0.0f);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use DNNL_ARG_DST SCALE for activation_scale since it will be applied after activation.

Comment on lines -113 to +159
attributes.set_output_scales(mask, output_shift_scale);
attributes.set_scales_mask(DNNL_ARG_DST, mask);
Copy link
Contributor

Choose a reason for hiding this comment

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

OUTPUT_SCALE is not equal to DST SCALE

@@ -37,37 +37,22 @@ class LayerNormOneDNNHandler
engine, cpu_place) {
const auto fwd_prop_kind = is_test ? dnnl::prop_kind::forward_inference
: dnnl::prop_kind::forward_training;

Copy link
Contributor

Choose a reason for hiding this comment

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

A random place:
Will these oneDNN kernels in fluid/operators/mkldnn dir be migrated to phi/kernels/onednn dir? Are we doing this task?

Copy link
Member

Choose a reason for hiding this comment

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

Hi, oneDNN kernels migration to PHI is finished. Kernels which are still in fluid are left on purpose (description: #45308)

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks for your explanation

@YangQun1
Copy link
Contributor

The convolution bias scaling process may be not needed anymore, since oneDNN v3.0 move the bias add after the dequantization. (Refer to the new quantization scheme: https://github.com/oneapi-src/oneDNN/tree/rfcs/rfcs/20220201-quantization-scaling)

@xinyu-intel
Copy link
Contributor

onednn v3.x has been upgraded in #52463. Thanks for the initial PR:)

@xinyu-intel xinyu-intel closed this Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers Intel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants