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

Snippets: precision propagation #14996

Merged

dmitry-gorokhov merged 6 commits into openvinotoolkit:master from eshoguli:es/snippets/precision_propagation_custom

Mar 23, 2023

Contributor

eshoguli commented Jan 9, 2023 •

edited

Loading

Details:

I32/I64 precision support: Snippets precision propagation

Tickets:

104281

github-actions bot added category: build category: CPU category: IE Tests category: LP transformations labels

ilya-lavrenov reviewed

View reviewed changes

src/common/snippets/CMakeLists.txt Outdated Show resolved Hide resolved

eshoguli force-pushed the es/snippets/precision_propagation_custom branch from 9f9b6f6 to d86c1d0 Compare

January 26, 2023 08:51

github-actions bot added category: Core and removed category: LP transformations labels

eshoguli force-pushed the es/snippets/precision_propagation_custom branch 2 times, most recently from 2610461 to ccba0c5 Compare

January 26, 2023 12:54

eshoguli changed the title ~~[DRAFT] Snippets: precision propagation~~ Snippets: precision propagation

eshoguli marked this pull request as ready for review

January 26, 2023 13:11

eshoguli requested review from a team as code owners

January 26, 2023 13:11

eshoguli assigned dmitry-gorokhov, IvanNovoselov, a-sidorova and v-Golubev

dmitry-gorokhov added this to the 2023.0 milestone

eshoguli force-pushed the es/snippets/precision_propagation_custom branch 7 times, most recently from f017c43 to cb23339 Compare

February 2, 2023 21:03

eshoguli force-pushed the es/snippets/precision_propagation_custom branch from cb23339 to 9ba9431 Compare

February 3, 2023 13:09

eshoguli force-pushed the es/snippets/precision_propagation_custom branch 4 times, most recently from 6ccf2ba to 165aff8 Compare

March 13, 2023 01:52

eshoguli requested review from IvanNovoselov and a-sidorova

March 13, 2023 02:01

eshoguli force-pushed the es/snippets/precision_propagation_custom branch 4 times, most recently from 9aae89f to de622d9 Compare

March 13, 2023 13:07

eshoguli added 3 commits

March 13, 2023 13:09


          Snippets: precision propagation

25a35d7


          check broadcast: tests were added

c18e791


          PropagatePrecision::get_precisions test

a-sidorova reviewed

View reviewed changes

src/common/snippets/tests/src/pass/precision_propagation_convert_test.cpp Outdated

		@@ -0,0 +1,153 @@
		// Copyright (C) 2022 Intel Corporation

Contributor

a-sidorova Mar 13, 2023

Minor

Suggested change

      
            // Copyright (C) 2022 Intel Corporation
          
            // Copyright (C) 2023 Intel Corporation

src/plugins/intel_cpu/src/snippets_transformations/remove_converts.hpp Outdated Show resolved Hide resolved

src/plugins/intel_cpu/src/snippets_transformations/remove_converts.cpp Outdated Show resolved Hide resolved

src/plugins/intel_cpu/src/transformation_pipeline.cpp Outdated Show resolved Hide resolved

eshoguli force-pushed the es/snippets/precision_propagation_custom branch from de622d9 to a12dc7c Compare

March 13, 2023 17:13


          Refactoring + comments

30dd8cb

a-sidorova approved these changes

View reviewed changes

IvanNovoselov requested changes

View reviewed changes

Contributor

IvanNovoselov left a comment

Unfortunately the tests are still not fixed despite numerous offline discussions. Please align tests in this PR with the rest of the snippets tests infrastructure. It's Ok if you suggest to refactor the infrastructure, but ,as was discussed with @dmitry-gorokhov, it should be done out of the scope of this PR.

src/plugins/intel_cpu/src/emitters/jit_emitter.hpp

+                  // returns supported precisions
+                  // precisions are ordered, the first bigger bitnes precision with the same type will be selected
+                  // empty collection means an emitter supports any input precisions
+                  static std::set<std::vector<element::Type>> get_supported_precisions(const std::shared_ptr<ngraph::Node>& node = nullptr);

Contributor

IvanNovoselov Mar 14, 2023

Example of documented methods here

src/plugins/intel_cpu/src/emitters/jit_snippets_emitters.cpp

+                      }
+                      default: {
+                          value = dnnl::impl::cpu::x64::float2int(ov::as_type_ptr<ov::op::v0::Constant>(n)->cast_vector<float>()[0]);
+                          break;

Contributor

IvanNovoselov Mar 14, 2023

Sorry, I didn't get your point, please elaborate

src/tests/ngraph_helpers/snippets_ngraph_functions/include/precision_propagation_function.hpp Outdated

+              namespace test {
+              namespace snippets {
+              class PrecisionPropagationFunction {

Contributor

IvanNovoselov Mar 14, 2023

Please remove this class, since we don't need it anymore

src/tests/ngraph_helpers/snippets_ngraph_functions/include/precision_propagation_function.hpp Outdated

+              class PrecisionPropagationFunction {
+              public:
+                  template<typename T>

Contributor

IvanNovoselov Mar 14, 2023

This template is instantiated only once, please convert it to an ordinary method.

src/tests/ngraph_helpers/snippets_ngraph_functions/include/precision_propagation_function.hpp Outdated

+                      const auto branch1 = make_branch(precision1, inputShape1, 1, convertion_before_op1.first);
+                      const auto branch2 = make_branch(precision2, inputShape2, 2, convertion_before_op1.second);
+                      std::shared_ptr<Node> parent = std::make_shared<T>(branch1.second, branch2.second);

Contributor

IvanNovoselov Mar 14, 2023

Minor: If you want to create different nodes in the future, you can just pass them as an argument. This way we can avoid duplication of the whole function body. We don't need this code duplication, since the function will be called just a couple times.

Contributor Author

eshoguli Mar 21, 2023

'If you want to create different nodes in the future, you can just pass them as an argument.'
Custom operation type is a part of function parameterization approach which is defined outside of function. With template it's more flexible.

'This way we can avoid duplication of the whole function body. We don't need this code duplication, since the function will be called just a couple times.'
In this case we mix function parameterization and function implementation, add if/else in function implementation. Template instantiation is not 'code dublication'.

Note, please, SnippetsFunctionBase and many of its useless empty inherited types with intermediate members increase binary size more then just static methods.

src/tests/ngraph_helpers/snippets_ngraph_functions/include/precision_propagation_function.hpp Outdated

+              };
+              template<typename T>
+              class EmpyInfrastructureSupportStubForPrecisionPropagationFunction : public SnippetsFunctionBase {

Contributor

IvanNovoselov Mar 14, 2023

Please provide a name that describes the function's topology, for example PrecisionPropagationAddFunction would do. You have a plenty good naming examples in the directory: ConvMulActivationFunction, TwoInputsAndOutputsFunction, BroadcastAddFunction

Contributor Author

eshoguli Mar 21, 2023

It's still template function, there is no Add operation in implementation. Will be renamed if template is removed.

src/tests/ngraph_helpers/snippets_ngraph_functions/include/precision_propagation_function.hpp Outdated

+                          actual.convertion_before_op2_2);
+                  }
+                  std::shared_ptr<ov::Model> initReference() const override {

Contributor

IvanNovoselov Mar 14, 2023

Why would you override different methods to yield the same function. Please, take a look at other ngraph snippets functions implementations here, they all override only initOriginal() since it's enough for functional tests

Contributor Author

eshoguli Mar 21, 2023

Because it's not plugin test when we need to infer one function only. You are reviewing functional tests with two function instances: original and reference which are compared after original function transformation.

...raph_helpers/snippets_ngraph_functions/include/precision_propagation_convertion_function.hpp Outdated

+              namespace test {
+              namespace snippets {
+              class PrecisionPropagationConvertionFunction {

Contributor

IvanNovoselov Mar 14, 2023

Couldn't find the answer below. Please align PrecisionPropagationFunction and PrecisionPropagationConvertionFunction with the rest of the snippets ngraph functions

src/common/snippets/src/pass/propagate_precision.cpp

Comment on lines +73 to +74

+                      assert(std::all_of(
+                          supported_precisions.begin(),
+                          supported_precisions.end(),
+                          [&input_precisions](const std::vector<element::Type>& precisions) {
+                              return precisions.size() == input_precisions.size();
+                          }) && "input precisions count is not equal for supported precisions");

Contributor

IvanNovoselov Mar 14, 2023

Ok, after an offline discussion I get your point regarding this particular assert and the one on L79. Let's leave them.

Didn't understand your argument regarding oneDNN or ONNX. It this a PR to a oneDNN repo?

IvanNovoselov reviewed

View reviewed changes

src/common/snippets/tests/src/pass/precision_propagation.cpp

+                          {{element::f32, element::f32}},
+                          {{element::f32, element::f32}}
+                      },
+                      {}

Contributor

IvanNovoselov Mar 14, 2023

Forgot to add the last comment:
You pass actual and expected to every test case, however expected is uniquely defined by actual, right?
It looks like it's enough to pass only actual and then produce both initial and reference functions. This logic could be implemented inside get_reference() method, for example. What do you think?

Contributor Author

eshoguli Mar 15, 2023

'You pass actual and expected to every test case, however expected is uniquely defined by actual, right?' - off course.

Nothing is easier to get model instance during development, debugging, extending and feature understanding, then the single static method which is parameterized with actual or expected data directly from a test case. For a new test addition, you just need to declare a new test case in the test_cases collection. No writing source code.

Let me explain:

To get model instance for the test we need specify some arguments for PrecisionPropagationFunction::get static method. To get two different model instances (original & expected) we should specify two sets of arguments (both are defined in the same test case) for the same PrecisionPropagationFunction::get static method. It's the simplest way to create two model instances (original & expected) for the test. Isn't it?
You suggested to keep the same PrecisionPropagationFunction::get method signature, the same function implementation but remove expected values from its test case and hardcode them for/in PrecisionPropagationFunction::get call? I can not agree with that, I didn't see any advantages of that.

Can you sugest anything more simplier for test understanding, debugging and extension in the future? May be we still leave one PrecisionPropagationFunction::get static method aproach ([template] customization will be removed) as is? : )))

a-sidorova mentioned this pull request

[Snippets] Added support of BF16/I8/U8 for MatMul #15063

Merged

2 tasks

eshoguli force-pushed the es/snippets/precision_propagation_custom branch 2 times, most recently from cffba04 to 1685d1a Compare

March 21, 2023 16:54

eshoguli requested a review from IvanNovoselov

March 21, 2023 16:59

eshoguli force-pushed the es/snippets/precision_propagation_custom branch from 17b1678 to 6ca4c3c Compare

March 22, 2023 01:23


          test: empty stub was added

ff90898

eshoguli force-pushed the es/snippets/precision_propagation_custom branch from 6ca4c3c to 1fdf05d Compare

March 22, 2023 01:29


          SnippetsFunctionBase virtualization issue fix

1fdf05d

IvanNovoselov approved these changes

View reviewed changes

dmitry-gorokhov approved these changes

View reviewed changes

src/common/snippets/src/pass/propagate_precision.cpp

+                  // general conditions: any new added precision will support
+                  return
+                      (actual.is_real() == required.is_real()) &&
+                      (actual.bitwidth() >= required.bitwidth());

Contributor

dmitry-gorokhov Mar 23, 2023

Maybe I missed smt but this condition will return true for: actual=bf16 and/or required fp16. Seems like this is invalid case for fusing.

dmitry-gorokhov merged commit 087b10f into openvinotoolkit:master

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

Reviewers

ilya-lavrenov ilya-lavrenov left review comments

IvanNovoselov IvanNovoselov approved these changes

v-Golubev v-Golubev approved these changes

a-sidorova a-sidorova approved these changes

dmitry-gorokhov dmitry-gorokhov approved these changes

Labels

category: Core category: CPU category: IE Tests category: Python API