[luci/pass] Introduce FuseMulWithFullyConnectedPass

[jiwaszki]

This commit introduce FuseMulWithFullyConnectedPass which will fuse Mul to previous FullyConnected if possible.

ONE-DCO-1.0-Signed-off-by: Jan Iwaszkiewicz [email protected]

[jiwaszki]

• For [one-optimize] Fuse mul with FC layer #13515
• From [one-optimize] Fuse Mul with FullyConnected layer #13528

[seanshpark]

@jiwaszki , can you reopen the draft PR and update it so that current full changes are passing the test?
with only this PR, it's not possible to know that end-to-end pass value test is working OK or not.

[seanshpark]

ditto

[seanshpark]

We usually go upward(to input).
For this case, begin with Mul and check one of the input is FC.
Can you please revise in this check order?

[seanshpark]

When you traverse from Mul to FC, FC input of Mul should be created and replaced to new FC as exiting FC may have multiple successors.

Current check only works when there exist single connection of FC-Mul.

[jiwaszki]

I see approach similar to mine on other pass which base on FC as well (with Add instead of Mul):

ONE/compiler/luci/pass/src/FuseAddWithFullyConnectedPass.cpp

Line 43 in 719a6f7

bool fuse_add_with_fc(luci::CircleFullyConnected *fc)

ONE/compiler/luci/pass/src/FuseAddWithFullyConnectedPass.cpp

Lines 58 to 61 in 719a6f7

	// Get add node
	auto fc_output = loco::succs(fc);
	if (fc_output.size() != 1)
	return false;

I want to make sure I understand the motivation here. Here is my understanding of a graph and the approach to pass you described:

In case of such pattern of single connection of FC-Mul, I understand the fusion of Mul into FC to eliminate node and reduce overall number of operations. However, if FC has more operations connected to it's output, the pass is simply replacing one op for another -- which from "math view" is resulting in the same count of operations.

Is that a matter of existing optimizations in targeted HW and/or more performant kernel implementations in ONE that FC is preferred here in place of Mul?

EDIT: I would understand a case where we look for Mul and check that connected FC has only one child and it's Mul itself. I wonder if that approach would work better as probably there are less Mul nodes to perform a check on than FC nodes (at least in popular models). I would need to have a better understanding of ONE method to applying passes to judge that. @seanshpark is that something that can be considered instead of adding multiple FC?

[seanshpark]

Is that a matter of existing optimizations in targeted HW and/or more performant kernel implementations in ONE that FC is preferred here in place of Mul?

No. I don't think so.

is that something that can be considered instead of adding multiple FC?

I cannot understand your question point.

[jinevening]

I see approach similar to mine on other pass which base on FC as well (with Add instead of Mul):

FuseAddWithFullyConnected fuses add with FC only when FC has a single successor (add). That is an intended behavior. If FC has multiple successors, FC layers are duplicated (as @jiwaszki said), which tends to increase model size and degrade model performance. I think FuseMulWithFullyConnected can follow the same strategy with FuseAddWithFullyConnected.

So, +1 for the current implementation.

[seanshpark]

I DISAGREE. I DO NOT WANT MULTIPLE WAYS OF SEARCHING.
SOME DAY FuseAddWithFullyConnected SHOULD BE FIXED TOO.

[jiwaszki]

@seanshpark How about the changes in 79a2213 ? It merges both ideas into one.
First, it follows the bottom-up approach, starting from mul:

ONE/compiler/luci/pass/src/FuseMulWithFullyConnectedPass.cpp

Line 110 in 79a2213

bool fuse_mul_with_fc(luci::CircleMul *mul)

ONE/compiler/luci/pass/src/FuseMulWithFullyConnectedPass.cpp

Lines 116 to 120 in 79a2213

	// Check if any FC node connects to Mul.
	// Find the pattern of Mul(FC, CircleConst):
	luci::CircleFullyConnected *fc = nullptr;
	luci::CircleConst *multiplication = nullptr;
	RETURN_FALSE_UNLESS(luci::fill(&fc, &multiplication).with_commutative_args_of(mul));

Second, still has the intended behavior of fusing only when there is only one successor to fc:

ONE/compiler/luci/pass/src/FuseMulWithFullyConnectedPass.cpp

Lines 121 to 122 in 79a2213

	// Make sure that FullyConnected has only one successor:
	RETURN_FALSE_UNLESS(loco::succs(fc).size() == 1);

@jinevening please also review the new commit when you have time.

[jinevening]

It looks good to me. Please address @seanshpark 's comments.

[jinevening]

LGTM

[seanshpark]

dtype check is duplicate with fuse_mul_with_fc().

[seanshpark]

Q) why use switch case with dtype here?

[jiwaszki]

It's in fact redundant check, leftover from refactoring. Removed and further simplified.

[seanshpark]

LGTM thank you!

[seanshpark]

For future readers for reference;

Q) Do we need to add RETURN_FALSE_UNLESS(loco::succs(fc).size() == 1); ?
A) No. This is optional and IMHO can benefit network connection case-by-case. Adding this can produce better network in performance like this PR but this is not confirmed for all cases. As our current implementation didn't had this line for most passes and no particular issue was raised.

Why am I leaving this comment?
Current implementation is like "we add conditions what is necessary and options are added when it is needed, that is when real models appear so that we can check and test that real conditions. what we can imagine with corner cases is OK but that imagination can grow without bound and also to the implementation."

I do not want to add "loco::succs(fc).size() == 1" to all existing implementations and future ones without real models, and without proved benefit with limited viewpoint.

[jiwaszki]

@jinevening pinging you just for formal approval. Thanks in advance!

[jinevening]

LGTM