-
Notifications
You must be signed in to change notification settings - Fork 183
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
[Gemmini] Fuse linalg.matmul and linalg.transpose when lowering to Gemmini DIalect #300
base: main
Are you sure you want to change the base?
Conversation
/*bScaleFactor = */ scale1, /*dScaleFactor = */ scale1, /*act = */ 0, | ||
/*accScale = */ scale1, /*bertScale = */ scale0); | ||
|
||
// llvm::outs() << " has " |
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.
Clear the comments here.
// If this matmul operation is followed by a transpose operation, do fusion. | ||
// We should make sure that the result of this matmul op only has one user. | ||
Operation* fuseOp = *output0.user_begin(); | ||
int output0Use = 0; |
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.
There is a serious error in logic here.I would suggest writing the pattern inside the canonicalizer of the gemmini.tile_matmul
op.What you should be determining is whether there is only one user of gemmini.tile_matmul
and it is linalg.transpose
.bTranspose
is a attribute, so let it be the opposite.You shouldn't be judging the number of linalg.transpose
, it's completely wrong to do so.A pass is based on a greedy algorithm, so this pass will continue to match against the pattern.
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.
Hi, thanks for your suggestions. I agree that it is indeed possible to do the match during gemmini.tile_matmul
lowering. However, I still don’t understand why there is a problem with greedy matching here. Can you give an example here? Maybe I’m not that familiar with it yet.
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.
%1 = gemmini.tile_matmul {transepose = false}
.....
%4 = linalg.transpose %1
.....
%7 = linalg.transpose %4
.....
use(%7)
first
%1 = gemmini.tile_matmul {transepose = true}
.....
%7 = linalg.transpose %1
.....
use(%7)
second
%1 = gemmini.tile_matmul {transepose = false}
.....
use(%1)
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.
%1 = gemmini.tile_matmul {transepose = false}
.....
%4 = linalg.transpose %1
.....
%7 = linalg.transpose %1
I'm not sure if this happens in the model not sure if it does.
So for this fuse pattern, you should make sure that there is only one op that uses gemmini.tile_matmul
and that op is linalg.transpose
.
} | ||
|
||
if (output0Use) { | ||
// llvm::outs() << "Fuse linalg.matmul and linalg.transpose. \n"; |
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.
Remove comments
/*bScaleFactor = */ scale1, /*dScaleFactor = */ scale1, /*act = */0, | ||
/*accScale = */ scale1, /*bertScale = */ scale0, | ||
/*repeatingBias = */ false, /*aTranspose = */ true, | ||
/*bTranspose = */ true); |
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.
bTranspose
is an attribute
.Obviously the method to create gemmini.tile_matmul
here just write one.
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.
For C = AB, C = C^T
, we use this formula : C^T = (AB)^T = B^T A^T
.
So, we change the position of input0
and input1
, and set aTranspose
and bTranspose
true.
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.
If so, the reasoning is the same, you just need to make aTranspose
the opposite of bTransepose
as before.
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.
There are a lot of problems here.
Regarding the conclusion that transpose is not supported, it is recommended that you write a C program to test it. The table is an indication of whether or not to transpose A and B. In this case, it is transpose C. |
As transposer is provied by Gemmini Arch, we can benefit from it when a matmul opearation is followed by a transpose operation. In this PR, we try to fuse
linalg.matmul
andlinalg.transpose
when lowering to Gemmini DIalect. And we make sure that the result of this matmul operation only has one user.However, when we use WS dataflow, A_transpose and B_transpose is not supported simultaneously. You can check this in https://github.com/ucb-bar/gemmini.
So we can compile succefully now, but may get incorrect result finally.