-
Notifications
You must be signed in to change notification settings - Fork 611
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
CDNA1/2 data tiling #19100
CDNA1/2 data tiling #19100
Conversation
d7bd67a
to
e335ba2
Compare
50c222c
to
ffc4d58
Compare
e335ba2
to
58beb25
Compare
ffc4d58
to
09d91a5
Compare
58beb25
to
a89bc41
Compare
Signed-off-by: Benoit Jacob <[email protected]>
09d91a5
to
ab2aa4c
Compare
Signed-off-by: Benoit Jacob <[email protected]>
a89bc41
to
e5899d6
Compare
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 did not check if the numbers match the spec. I like the size of the PR when we add the support for CDNA1/2 data-tiling. :)
@@ -1741,6 +1741,36 @@ iree_generated_e2e_runner_test( | |||
"requires-gpu-cdna3" | |||
) | |||
|
|||
iree_generated_e2e_runner_test( | |||
NAME | |||
e2e_matmul_cdna3_dt_f64 |
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'm confused that a cdna3 test suite is added but the PR title is about CDNA1/2. Why?
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.
Sorry, that's just a mishap while slicing my diff into PRs. This was meant to be part of #19099.
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, can you rebase the PR (or merge the main branch into your branch)?
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.
@hanhanW , if I do that, I will lose the green CI status. The bottom commit here is already in main. Can you review based on looking at the top commit?
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.
ah, I see. I did not notice that there are two commits. Reviewed and it looks good to me, thanks!
CDNA1/2 machines are going to be in use for a while, and adding data tiling support for an architecture is just a matter of populating those 3 optional fields in
TargetWgpDetails
, and the correspondinggpu_materialize_encoding_gfx***.mlir
test adds some coverage around the intrinsics that is useful beyond data tiling.