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.

Sign up for GitHub

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

Jump to bottom

[PHI] Sum op migration #46239

Merged

jczaja merged 9 commits into PaddlePaddle:develop from paulinagacek:sum_operator_migration

Sep 22, 2022

Contributor

paulinagacek commented Sep 19, 2022

PR types

Others

PR changes

Others

Describe

Migrate sum(add) operator to phi

paddle-bot bot commented Sep 19, 2022

你的PR提交成功，感谢你对开源项目的贡献!
请关注后续CI自动化测试结果，详情请参考Paddle-CI手册。
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

CLAassistant commented Sep 19, 2022 •

edited

Loading

All committers have signed the CLA.

paddle-bot bot added contributor status: proposed labels

paulinagacek added 3 commits

September 19, 2022 11:24


          Sum kernel migrated to phi

42d6886


          Static cast added, file name changed

33fe9e5


          Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

769a083

… sum_operator_migration

paulinagacek marked this pull request as ready for review

September 20, 2022 19:08

paulinagacek added 2 commits

September 20, 2022 21:01


          Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

7558e78

… sum_operator_migration


          OneDNNGetDataType to uppercase

f7b9299

Contributor Author

paulinagacek commented Sep 21, 2022

@piotrekobi, @Silv3S can you review please?

Silv3S mentioned this pull request

Migrate oneDNN operators to under PHI #45308

Closed

Silv3S added the Intel label

paddle-bot bot removed the status: proposed label

Silv3S previously approved these changes

View reviewed changes

Member

Silv3S left a comment

I left a few minor comments but overall a very good job
LGTM 👍

paddle/phi/kernels/onednn/add_n_kernel.cc Outdated

+                AddOneDNNHandler(dnnl::engine engine,
+                                 const phi::Place& cpu_place,
+                                 const std::vector<const TensorBase*>& x,
+                                 DenseTensor* z)

Member

Silv3S Sep 21, 2022

maybe change it to out because z is unintuitive

paddle/phi/kernels/onednn/add_n_kernel.cc Outdated

+                PADDLE_ENFORCE_EQ(
+                    dev_ctx.GetPlace().GetType() == phi::AllocationType::CPU,
+                    true,
+                    phi::errors::PreconditionNotMet("Operator DNNL Sum must use CPUPlace"));

Member

Silv3S Sep 21, 2022

DNNL is deprecated, please replace with oneDNN.
I'd change Operator DNNL Sum either to oneDNN Sum primitive or oneDNN AddN kernel.

paddle/phi/kernels/onednn/add_n_kernel.cc Outdated

+              namespace phi {
+              namespace funcs {
+              template <typename T>
+              class AddOneDNNHandler

Member

Silv3S Sep 21, 2022

We usually name handlers after oneDNN primitive names. For example scale or gelu kernels use ActivationKernel, because oneDNN treats such linear operations as activations.

Contributor Author

paulinagacek Sep 21, 2022

So what name do you suggest instead?

Member

Silv3S Sep 21, 2022

SumOneDNNHandler

paddle/phi/kernels/onednn/add_n_kernel.cc Outdated

Comment on lines 123 to 127

+                if (in_place) {
+                  dst_mem = srcs_mem[0];
+                } else {
+                  dst_mem = handler.AcquireDstMemory(out);
+                }

Member

Silv3S Sep 21, 2022

auto dst_mem = in_place ? srcs_mem[0] : handler.AcquireDstMemory(out);

piotrekobi reviewed

View reviewed changes

Contributor

piotrekobi left a comment

Please remove the unnecessary namespace names. Great job once again :D!

paddle/phi/kernels/onednn/add_n_kernel.cc Outdated

+              namespace funcs {
+              template <typename T>
+              class AddOneDNNHandler
+                  : public phi::funcs::OneDNNHandlerNoCachingT<T, dnnl::sum> {

Contributor

piotrekobi Sep 21, 2022

Suggested change

      
                : public phi::funcs::OneDNNHandlerNoCachingT<T, dnnl::sum> {
          
                : public OneDNNHandlerNoCachingT<T, dnnl::sum> {

paddle/phi/kernels/onednn/add_n_kernel.cc Outdated

+                                 const std::vector<const TensorBase*>& x,
+                                 DenseTensor* z)
+                    : phi::funcs::OneDNNHandlerNoCachingT<T, dnnl::sum>(engine, cpu_place),

Contributor

piotrekobi Sep 21, 2022

Suggested change

      
                  : phi::funcs::OneDNNHandlerNoCachingT<T, dnnl::sum>(engine, cpu_place),
          
                  : OneDNNHandlerNoCachingT<T, dnnl::sum>(engine, cpu_place),

paddle/phi/kernels/onednn/add_n_kernel.cc Outdated

+                    : phi::funcs::OneDNNHandlerNoCachingT<T, dnnl::sum>(engine, cpu_place),
+                      num_inputs_(0) {
+                  auto dst_tz = phi::vectorize<int64_t>(z->dims());

Contributor

piotrekobi Sep 21, 2022

Suggested change

      
                auto dst_tz = phi::vectorize<int64_t>(z->dims());
          
                auto dst_tz = vectorize<int64_t>(z->dims());

paddle/phi/kernels/onednn/add_n_kernel.cc Outdated

+                                                          to_void_cast<T>(input_data));
+                }
+                using phi::funcs::OneDNNHandlerNoCachingT<T, dnnl::sum>::AcquireDstMemory;

Contributor

piotrekobi Sep 21, 2022

Suggested change

      
              using phi::funcs::OneDNNHandlerNoCachingT<T, dnnl::sum>::AcquireDstMemory;
          
              using OneDNNHandlerNoCachingT<T, dnnl::sum>::AcquireDstMemory;

paddle/phi/kernels/onednn/add_n_kernel.cc Outdated

+                              const std::vector<const TensorBase*>& x,
+                              DenseTensor* out) {
+                PADDLE_ENFORCE_EQ(
+                    dev_ctx.GetPlace().GetType() == phi::AllocationType::CPU,

Contributor

piotrekobi Sep 21, 2022

Suggested change

      
                  dev_ctx.GetPlace().GetType() == phi::AllocationType::CPU,
          
                  dev_ctx.GetPlace().GetType() == AllocationType::CPU,

paddle/phi/kernels/onednn/add_n_kernel.cc Outdated

+                PADDLE_ENFORCE_EQ(
+                    dev_ctx.GetPlace().GetType() == phi::AllocationType::CPU,
+                    true,
+                    phi::errors::PreconditionNotMet("Operator DNNL Sum must use CPUPlace"));

Contributor

piotrekobi Sep 21, 2022

Suggested change

      
                  phi::errors::PreconditionNotMet("Operator DNNL Sum must use CPUPlace"));
          
                  errors::PreconditionNotMet("Operator DNNL Sum must use CPUPlace"));

paddle/phi/kernels/onednn/add_n_kernel.cc Outdated

+                PADDLE_ENFORCE_NE(x.empty(),
+                                  true,
+                                  phi::errors::InvalidArgument("Input variable is empty."));

Contributor

piotrekobi Sep 21, 2022

Suggested change

      
                                phi::errors::InvalidArgument("Input variable is empty."));
          
                                errors::InvalidArgument("Input variable is empty."));

paulinagacek dismissed Silv3S’s stale review via

efc301a

September 21, 2022 12:51

paulinagacek added 4 commits

September 21, 2022 14:45


          refactoring

410cd2a


          Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

efc301a

… sum_operator_migration


          AddOneDNNHandler changed to SumOneDNNHandler

8b317c0


          Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

9b3e7f8

… sum_operator_migration

piotrekobi approved these changes

View reviewed changes

Contributor

piotrekobi left a comment

LGTM

Contributor Author

paulinagacek commented Sep 22, 2022

@jczaja I think it is ready to be merged

jczaja approved these changes

View reviewed changes

Contributor

jczaja left a comment

LGTM

jczaja merged commit 3448afc into PaddlePaddle:develop

paddle-bot bot added the status: accepted label

paddle-bot bot commented Sep 22, 2022

你的PR已合入Paddle库，请关注后续测试结果。
Your PR has been merged into the repository. An official integration test will be conducted later. Stay tuned.

umiswing pushed a commit to umiswing/Paddle that referenced this pull request


          [PHI] Sum op migration (PaddlePaddle#46239)

708792d

* Sum kernel migrated to phi

* Static cast added, file name changed

* OneDNNGetDataType to uppercase

* refactoring

* AddOneDNNHandler changed to SumOneDNNHandler

Silv3S pushed a commit to Silv3S/Paddle that referenced this pull request


          [PHI] Sum op migration (PaddlePaddle#46239)

4e2478a

* Sum kernel migrated to phi

* Static cast added, file name changed

* OneDNNGetDataType to uppercase

* refactoring

* AddOneDNNHandler changed to SumOneDNNHandler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor Intel