Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

qzylalala
Copy link

@qzylalala qzylalala commented May 8, 2024

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 and linalg.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.

As of now, certain combinations of transpose options cannot be performed unless the right dataflow is chosen. This limitation may be lifted in the future.

So we can compile succefully now, but may get incorrect result finally.

@qzylalala qzylalala changed the title [Gemmini] Fuse [Gemmini] Fuse linalg.matmul and linalg.transpose when lowering to Gemmini DIalect May 8, 2024
/*bScaleFactor = */ scale1, /*dScaleFactor = */ scale1, /*act = */ 0,
/*accScale = */ scale1, /*bertScale = */ scale0);

// llvm::outs() << " has "
Copy link
Member

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;
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

@linuxlonelyeagle linuxlonelyeagle May 10, 2024

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)

Copy link
Member

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";
Copy link
Member

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);
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Member

@linuxlonelyeagle linuxlonelyeagle left a 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.

@linuxlonelyeagle
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants