[one-optimize] Fuse Mul with FullyConnected layer #13528

jiwaszki · 2024-07-26T15:02:50Z

This commit introduces fuse_mul_with_fully_connected pass that combines FullyConnected and Mul into one node.

ONE-DCO-1.0-Signed-off-by: Jan Iwaszkiewicz j.iwaszkiewi@samsung.com

This commit introduces fuse_mul_with_fully_connected pass that combines FullyConnected and Mul into one node. ONE-DCO-1.0-Signed-off-by: Jan Iwaszkiewicz <j.iwaszkiewi@samsung.com>

jiwaszki · 2024-07-26T15:08:41Z

Addresses the issue #13515
Changes from PR were tested on custom made model with same dims as mentioned in the issue.

TODO: add testcases and cover possible edge-cases.

@jinevening please take a look when you have some time, all feedback is highly appreciated (especially about optimization rules/guidelines in the project, i.e. rules regarding usage of dynamic_cast/must_cast and returning false/fail checks). Thanks in advance!

seanshpark · 2024-07-28T21:35:55Z

compiler/luci/pass/src/CircleOptimizer.cpp

@@ -42,6 +42,7 @@
 #include "luci/Pass/FuseBatchNormWithDwConvPass.h"
 #include "luci/Pass/FuseBatchNormWithTConvPass.h"
 #include "luci/Pass/FuseBCQPass.h"
+#include "luci/Pass/FuseMulWithFullyConnectedPass.h"


plz move this line below #include "luci/Pass/FuseMulWithDivPass.h"

seanshpark · 2024-07-28T21:36:25Z

compiler/luci/pass/include/luci/CircleOptimizer.h

@@ -41,6 +41,7 @@ class CircleOptimizer final
      FuseBatchNormWithConv,
      FuseBatchNormWithDwConv,
      FuseBatchNormWithTConv,
+      FuseMulWithFullyConnected,


plz move this line below FuseMulWithDiv,

seanshpark · 2024-07-28T21:36:53Z

compiler/circle2circle/src/Circle2Circle.cpp

+  if (arser.get<bool>("--fuse_mul_with_fully_connected"))
+    options->enable(Algorithms::FuseMulWithFullyConnected);


plz move below

if (arser.get("--fuse_mul_with_div"))
options->enable(Algorithms::FuseMulWithDiv);

seanshpark · 2024-07-28T21:37:18Z

compiler/circle2circle/src/Circle2Circle.cpp

@@ -112,6 +112,8 @@ int entry(int argc, char **argv)
  add_switch(arser, "--fuse_mean_with_mean",
             "This will fuse two Mean operations when they follow one by one. This will fold them "
             "into one operation and merge reduction indices.");
+  add_switch(arser, "--fuse_mul_with_fully_connected",


plz move below add_switch(arser, "--fuse_mul_with_div",

seanshpark · 2024-07-28T21:38:18Z

compiler/luci/pass/src/FuseMulWithFullyConnectedPass.cpp

@@ -0,0 +1,183 @@
+/*


plz add FuseMulWithFullyConnectedPass.test.cpp

in the unit test, you should add count(positive) <= count(negative) where negative method name ends with _NEG

jinevening · 2024-07-29T04:51:08Z

compiler/luci/pass/src/FuseMulWithFullyConnectedPass.cpp

+  RETURN_FALSE_UNLESS(weights->opcode() == luci::CircleOpcode::CIRCLECONST or
+                      weights->opcode() == luci::CircleOpcode::CIRCLEOUTPUTEXCLUDE)


weights is always const.

Done in scope of 4c7f5a9

jinevening · 2024-07-29T04:55:21Z

compiler/luci/pass/src/FuseMulWithFullyConnectedPass.cpp

+  update_values(fused_weights, multiplication);
+
+  fc->weights(fused_weights);


Please check all conditions at first & update at last.

If you incrementally update a node like this, fc can be invalid in some cases. For example, if bias is non-const, fc's weights will be updated, but that conversion would be wrong.

Done in cfcb68b

seanshpark · 2024-07-29T05:06:53Z

For validation, plz add or edit

res/TensorFlowLiteRecipes/Net_FullyConnected_Mul_000 and more for testing the pass
- test.recipe for source model
- test.rule for dredd test after the pass
compiler/luci-pass-value-py-test/test.lst
- add Net_FullyConnected_Mul_000 and others with optimize option name
compiler/circle2circle-dredd-recipe-test/test.lst
- add Net_FullyConnected_Mul_000 and others

seanshpark · 2024-08-04T21:36:28Z

compiler/circle2circle/src/Circle2Circle.cpp

  add_switch(arser, "--fuse_mul_to_fullyconnected_weights",
             "This will fuse Mul to following FullyConnected weights");
  add_switch(arser, "--fuse_mul_with_conv",
             "This will fuse Mul operation with a preceding Conv if possible.");
  add_switch(arser, "--fuse_mul_with_div",
             "This will fuse Mul operation with a Div operation whose numerator is const.");
+  add_switch(arser, "--fuse_mul_with_fully_connected",


Suggested change

add_switch(arser, "--fuse_mul_with_fully_connected",

add_switch(arser, "--fuse_mul_with_fullyconnected",

to follow other options

Got it! Done in scope of d386541

jiwaszki · 2024-08-05T13:55:09Z

@seanshpark @jinevening I would ask for another review on this PR. I really appreciate your help!

I have a question regarding possible merging of the PR. If approved, should I split it into smaller ones like "[luci] Fuse Mul with FC"/"[tests] Fuse Mul with FC" or keep it as-is?

seanshpark · 2024-08-05T21:36:00Z

... should I split it into smaller ones like ...

yes, please separate compiler/* and
it would be better to put single context of change in each PRs

jinevening · 2024-08-06T02:07:33Z

compiler/luci/pass/src/FuseMulWithFullyConnectedPass.cpp

+  RETURN_FALSE_UNLESS(fused_weights->size<loco::DataType::FLOAT32>() ==
+                      weights->size<loco::DataType::FLOAT32>());


Seems unnecessary.

jinevening · 2024-08-06T02:07:39Z

compiler/luci/pass/src/FuseMulWithFullyConnectedPass.cpp

+  RETURN_FALSE_UNLESS(fused_bias->size<loco::DataType::FLOAT32>() ==
+                      const_bias->size<loco::DataType::FLOAT32>());


jinevening · 2024-08-06T02:11:03Z

compiler/luci/pass/src/FuseMulWithFullyConnectedPass.cpp

+inline void update_values(luci::CircleConst *fused_node, luci::CircleConst *multiplication,
+                          bool is_weights)


Could you split this to update_weights and update_bias ?

jinevening · 2024-08-06T02:15:06Z

There are some minor comments, but looks good in overall. Please proceed after reading @seanshpark 's comment (#13528 (comment)).

This commit introduce FuseMulWithFullyConnectedPass which will fuse Mul to previous FullyConnected if possible. ONE-DCO-1.0-Signed-off-by: Jan Iwaszkiewicz <j.iwaszkiewi@samsung.com>

This commit adds one-cmd option for FuseMulWithFullyConnectedPass. ONE-DCO-1.0-Signed-off-by: Jan Iwaszkiewicz <j.iwaszkiewi@samsung.com>

This commit is adding circle2circle dredd test for FC + Mul fusion. ONE-DCO-1.0-Signed-off-by: Jan Iwaszkiewicz <j.iwaszkiewi@samsung.com>

jiwaszki · 2024-08-09T15:21:50Z

@seanshpark I have merged other branches here. Now let's wait for CIs to pass.

jiwaszki · 2024-08-09T16:10:55Z

@nnfw-bot test nncc-release

This commit extends Net_FullyConnectedMul tflite recipes with 'no bias' case. ONE-DCO-1.0-Signed-off-by: Jan Iwaszkiewicz <j.iwaszkiewi@samsung.com>

…e_mul_fc

jiwaszki · 2024-08-26T14:57:30Z

All related PRs from #13515 are closed. Closing this draft as well.

[one-optimize] Fuse Mul with FullyConnected layer

28d8089

This commit introduces fuse_mul_with_fully_connected pass that combines FullyConnected and Mul into one node. ONE-DCO-1.0-Signed-off-by: Jan Iwaszkiewicz <j.iwaszkiewi@samsung.com>

seanshpark reviewed Jul 28, 2024

View reviewed changes

jinevening reviewed Jul 29, 2024

View reviewed changes

jiwaszki added 5 commits July 29, 2024 16:26

Move mul_with_fully_connected pass after the mul_with_div

87774c5

Remove weights constant check

4c7f5a9

Change order of updating the nodes, more consuming one is now later

cfcb68b

Fix values updating and add luci tests

e8b06b5

Fix codestyle

3bf2649

seanshpark reviewed Aug 4, 2024

View reviewed changes

jiwaszki added 3 commits August 5, 2024 11:33

Rename pass

d386541

Add luci tests with models

4180734

Fix scalar vs multi-dim case

761d303

jiwaszki marked this pull request as ready for review August 5, 2024 13:48

jinevening reviewed Aug 6, 2024

View reviewed changes

jiwaszki added 4 commits August 6, 2024 16:11

Separate bias and weights updating, remove checks

fa4733b

[luci/pass] Introduce FuseMulWithFullyConnectedPass

40dcacf

This commit introduce FuseMulWithFullyConnectedPass which will fuse Mul to previous FullyConnected if possible. ONE-DCO-1.0-Signed-off-by: Jan Iwaszkiewicz <j.iwaszkiewi@samsung.com>

[one-cmds] Add an option for FuseMulWithFullyConnectedPass

b717234

This commit adds one-cmd option for FuseMulWithFullyConnectedPass. ONE-DCO-1.0-Signed-off-by: Jan Iwaszkiewicz <j.iwaszkiewi@samsung.com>

[circle2circle] Dredd test for FuseMulWithFullyConnectedPass

a568d25

This commit is adding circle2circle dredd test for FC + Mul fusion. ONE-DCO-1.0-Signed-off-by: Jan Iwaszkiewicz <j.iwaszkiewi@samsung.com>

jiwaszki added 4 commits August 9, 2024 16:32

Merge branch 'jiwaszki/fuse_mul_fc_one_cmds' into jiwaszki/fuse_mul_fc

d5ec1d8

Add handling of no bias case to pass

8b17f47

Merge branch 'jiwaszki/fuse_mul_fc_luci_pass' into jiwaszki/fuse_mul_fc

8d90e50

Remove random newline

715cdf7

jiwaszki added 22 commits August 12, 2024 13:12

Apply comments, refactor tests and add proper handling of OUTPUTEXCLUDE

9e22b26

Merge branch 'jiwaszki/fuse_mul_fc_luci_pass' into jiwaszki/fuse_mul_fc

62a09a0

Resolve one-cmds duplication

1b6c71f

Handle rank 0 and 1

8977ef9

Merge branch 'jiwaszki/fuse_mul_fc_luci_pass' into jiwaszki/fuse_mul_fc

dbed1b9

Add new testcase

53aa943

Add new testcase

0c2bb71

[res/tfl_recipes] Add new Net_FullyConnected_Mul

e3ff517

This commit extends Net_FullyConnectedMul tflite recipes with 'no bias' case. ONE-DCO-1.0-Signed-off-by: Jan Iwaszkiewicz <j.iwaszkiewi@samsung.com>

Merge branch 'jiwaszki/fuse_mul_fc_new_tfl_recipes' into jiwaszki/fus…

d6e8b4a

…e_mul_fc

Merge branch 'jiwaszki/fuse_mul_fc_c2c_dredd' into jiwaszki/fuse_mul_fc

b4ebd44

Merge branch 'jiwaszki/fuse_mul_fc_luci_test' into jiwaszki/fuse_mul_fc

cddd353

Change name of operand from B to scale

678869c

Merge branch 'jiwaszki/fuse_mul_fc_new_tfl_recipes' into jiwaszki/fus…

af3119d

…e_mul_fc

Update names from scalar to single element

b085181

Update tests

1aa79cc

Merge branch 'jiwaszki/fuse_mul_fc_luci_pass' into jiwaszki/fuse_mul_fc

f02cb88

Fix codestyle

1bb278d

Merge branch 'jiwaszki/fuse_mul_fc_luci_pass' into jiwaszki/fuse_mul_fc

27dec03

Search from mul, update tests

79a2213

Merge branch 'jiwaszki/fuse_mul_fc_luci_pass' into jiwaszki/fuse_mul_fc

7ea759a

Annotate requirement of one successor and refactor checks

550e798

Merge branch 'jiwaszki/fuse_mul_fc_luci_pass' into jiwaszki/fuse_mul_fc

bda96d8

jiwaszki closed this Aug 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[one-optimize] Fuse Mul with FullyConnected layer #13528

[one-optimize] Fuse Mul with FullyConnected layer #13528

jiwaszki commented Jul 26, 2024

jiwaszki commented Jul 26, 2024

seanshpark Jul 28, 2024

seanshpark Jul 28, 2024

seanshpark Jul 28, 2024

seanshpark Jul 28, 2024

seanshpark Jul 28, 2024

seanshpark Jul 28, 2024

jinevening Jul 29, 2024

jiwaszki Aug 5, 2024

jinevening Jul 29, 2024

jiwaszki Aug 5, 2024

seanshpark commented Jul 29, 2024

seanshpark Aug 4, 2024

jiwaszki Aug 5, 2024

jiwaszki commented Aug 5, 2024

seanshpark commented Aug 5, 2024

jinevening Aug 6, 2024

jinevening Aug 6, 2024

jinevening Aug 6, 2024

jinevening commented Aug 6, 2024

jiwaszki commented Aug 9, 2024

jiwaszki commented Aug 9, 2024

jiwaszki commented Aug 26, 2024

		if (arser.get<bool>("--fuse_mul_with_fully_connected"))
		options->enable(Algorithms::FuseMulWithFullyConnected);

		RETURN_FALSE_UNLESS(weights->opcode() == luci::CircleOpcode::CIRCLECONST or
		weights->opcode() == luci::CircleOpcode::CIRCLEOUTPUTEXCLUDE)

		update_values(fused_weights, multiplication);

		fc->weights(fused_weights);

	add_switch(arser, "--fuse_mul_with_fully_connected",
	add_switch(arser, "--fuse_mul_with_fullyconnected",

		RETURN_FALSE_UNLESS(fused_weights->size<loco::DataType::FLOAT32>() ==
		weights->size<loco::DataType::FLOAT32>());

		RETURN_FALSE_UNLESS(fused_bias->size<loco::DataType::FLOAT32>() ==
		const_bias->size<loco::DataType::FLOAT32>());

		inline void update_values(luci::CircleConst fused_node, luci::CircleConst multiplication,
		bool is_weights)

[one-optimize] Fuse Mul with FullyConnected layer #13528

[one-optimize] Fuse Mul with FullyConnected layer #13528

Conversation

jiwaszki commented Jul 26, 2024

jiwaszki commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanshpark commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiwaszki commented Aug 5, 2024

seanshpark commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinevening commented Aug 6, 2024

jiwaszki commented Aug 9, 2024

jiwaszki commented Aug 9, 2024

jiwaszki commented Aug 26, 2024