Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[luci/pass] Introduce FuseMulWithFullyConnectedPass #13607
[luci/pass] Introduce FuseMulWithFullyConnectedPass #13607
Changes from 1 commit
40dcacf
f661561
85d9783
e3b354e
31e25ed
8b17f47
9e22b26
8977ef9
b085181
1aa79cc
1bb278d
79a2213
550e798
48defc2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually go upward(to input).
For this case, begin with
Mul
and check one of the input isFC
.Can you please revise in this check order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you traverse from
Mul
toFC
,FC
input ofMul
should be created and replaced to newFC
as exitingFC
may have multiple successors.Current check only works when there exist single connection of
FC
-Mul
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see approach similar to mine on other pass which base on
FC
as well (withAdd
instead ofMul
):ONE/compiler/luci/pass/src/FuseAddWithFullyConnectedPass.cpp
Line 43 in 719a6f7
ONE/compiler/luci/pass/src/FuseAddWithFullyConnectedPass.cpp
Lines 58 to 61 in 719a6f7
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 ofMul
intoFC
to eliminate node and reduce overall number of operations. However, ifFC
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 ofMul
?EDIT: I would understand a case where we look for
Mul
and check that connectedFC
has only one child and it'sMul
itself. I wonder if that approach would work better as probably there are lessMul
nodes to perform a check on thanFC
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 multipleFC
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I don't think so.
I cannot understand your question point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thinkFuseMulWithFullyConnected
can follow the same strategy withFuseAddWithFullyConnected
.So, +1 for the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I DISAGREE. I DO NOT WANT MULTIPLE WAYS OF SEARCHING.
SOME DAY FuseAddWithFullyConnected SHOULD BE FIXED TOO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
ONE/compiler/luci/pass/src/FuseMulWithFullyConnectedPass.cpp
Lines 116 to 120 in 79a2213
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
@jinevening please also review the new commit when you have time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me. Please address @seanshpark 's comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dtype
check is duplicate withfuse_mul_with_fc()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q) why use switch case with dtype here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in fact redundant check, leftover from refactoring. Removed and further simplified.