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.

Sign up for GitHub

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

Jump to bottom

[DispatchCreation] Make truncate operations fuse with producers. #19847

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

[DispatchCreation] Make truncate operations fuse with producers. #19847

wants to merge 4 commits into from

Conversation

MaheshRavishankar
Copy link
Contributor

No description provided.

@IanWood1 IanWood1 force-pushed the truncate_with_producer branch 2 times, most recently from 2ced301 to cebecc1 Compare January 29, 2025 21:44
@IanWood1
Copy link
Contributor

IanWood1 commented Jan 30, 2025 •
edited
Loading

https://gist.github.com/IanWood1/0aceb30d6fc0a5a99ccca68e9eac6cba from fp16 unet is still a problem, and takes 2ms (~500ms total) when it should be in the single digit microsecond range

Edit: for some reason this change is causing more horizontal fusion to kick in leading to this bad dispatch

@IanWood1 IanWood1 force-pushed the truncate_with_producer branch 2 times, most recently from 6d767f6 to 2dbb8a5 Compare January 30, 2025 03:25
@MaheshRavishankar
Copy link
Contributor Author

Could you file an issue for the changed dispatch. @nirvedhmeshram can you help fixing that

@nirvedhmeshram
Copy link
Contributor

nirvedhmeshram commented Jan 30, 2025 •
edited
Loading

The layout is pretty bad for this dispatch it is (M,K) * (B, N, K) -> (B, M, N). I can explore if we can improve perf for this, or should we be trying to make this (B,M,K) * (N, K) -> (B, M, N) followed by a transpose to make that (B, N, M) before dispatch creation?

@qedawkins
Copy link
Contributor

Why is (M,K) * (B, N, K) -> (B, M, N) a poor layout? This looks close to a typical matmul_transpose_b to me.

@nirvedhmeshram
Copy link
Contributor

Why is (M,K) * (B, N, K) -> (B, M, N) a poor layout? This looks close to a typical matmul_transpose_b to me.

I figured we would rather have the batch dimension on the lhs, rather than the the rhs, but you are right it shouldnt matter. Let me dig deeper if its just something in our default config.

@nirvedhmeshram
Copy link
Contributor

Currently this going though tile-and-fuse vectorize path, I even relaxed the limit of 4 we have to classify as matvec and it still gave

[iree-gpu-config-utils]: Matmul TileAndFuse Config
[iree-gpu-config-utils]: Attempting to deduce unaligned TileAndFuse MMA schedulee
[iree-gpu-config-utils]: Failed to deduce TileAndFuse MMA schedule

@IanWood1 IanWood1 mentioned this pull request Jan 30, 2025
Closed
@nirvedhmeshram
Copy link
Contributor

Attempting to deduce unaligned TileAndFuse MMA schedulee

Sorry I made a bug in relaxing this, when done correctly we get, 0.068 ms vs the default path which gets 1.64 ms

MaheshRavishankar added a commit to MaheshRavishankar/iree that referenced this pull request Jan 30, 2025
@MaheshRavishankar
[DispatchCreation] Allow horizontal fusion when all contractions feed…
833980d 
… to same trunc operator.

Currently the fusion does not always fuse truncates with its producers
(that is being fixed in iree-org#19847),
but without that the truncate operators could be the same. Allow
horizontal fusion to work with that case.

Signed-off-by: MaheshRavishankar <mahesh.ravishankar@gmail.com>
@MaheshRavishankar MaheshRavishankar mentioned this pull request Jan 30, 2025
Closed
@IanWood1
Copy link
Contributor

@nirvedhmeshram could you ping me when you have a branch, I want to bench that change + re-enable horizontal fusion.

@nirvedhmeshram
Copy link
Contributor

@nirvedhmeshram could you ping me when you have a branch, I want to bench that change + re-enable horizontal fusion.

@IanWood1 PR is ready #19857 you need the flag --iree-codegen-llvmgpu-test-tile-and-fuse-matmul=true,

This was referenced Jan 30, 2025
Open
Open
Merged
@IanWood1 IanWood1 force-pushed the truncate_with_producer branch from a14525e to 9210919 Compare February 4, 2025 17:06
MaheshRavishankar and others added 4 commits February 5, 2025 13:22
@MaheshRavishankar @IanWood1
[DispatchCreation] Make truncate operations fuse with producers.
fb396dd 
Signed-off-by: MaheshRavishankar <mravisha@amd.com>
@IanWood1
Fix fusion logic and allow fusing conv ops with truncate consumers
9e0f496 
Signed-off-by: Ian Wood <ianwood2024@u.northwestern.edu>
@IanWood1
Add pipeline test for trunc fusion
05beeb3 
Signed-off-by: Ian Wood <ianwood2024@u.northwestern.edu>
@IanWood1
Reorder so that gather pattern is applied first
fbbeaf4 
Signed-off-by: Ian Wood <ianwood2024@u.northwestern.edu>
@IanWood1 IanWood1 force-pushed the truncate_with_producer branch from 9210919 to fbbeaf4 Compare February 5, 2025 20:15
@IanWood1 IanWood1 mentioned this pull request Feb 14, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanhanW hanhanW Awaiting requested review from hanhanW hanhanW will be requested when the pull request is marked ready for review hanhanW is a code owner

@IanWood1 IanWood1 Awaiting requested review from IanWood1 IanWood1 will be requested when the pull request is marked ready for review IanWood1 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MaheshRavishankar @IanWood1 @nirvedhmeshram @qedawkins