-
Notifications
You must be signed in to change notification settings - Fork 604
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
e2e matmul test improvements #18725
e2e matmul test improvements #18725
Conversation
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
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.
Nice, that cmake file was a lot to navigate. I don't have much context about the cpu tests so maybe would be good to have a stamp from someone who does.
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
@kuhar , can you review particularly the last commit here (changing the large test shapes, which were folded from the gpu_large shapes in another commit on this PR)? Based on @qedawkins 's comment, I changed the shapes a little to increase diversity. |
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.
@kuhar , can you review particularly the last commit here (changing the large test shapes, which were folded from the gpu_large shapes in another commit on this PR)? Based on @qedawkins 's comment, I changed the shapes a little to increase diversity.
Looks fine to me, although these 'LARGE' cases are pretty small for modern GPUs. But I guess that's desired for in-repo tests.
Signed-off-by: Benoit Jacob <jacob.benoit.1@gmail.com>
Yes, there's a trade-off here. We could have an "XL" shapes set if at some point we think that some bugs are likely to be only caught with such shapes. We would just have to be careful to only use it on powerful targets such as GPU. |
@bjacob what's the accumulator type in bf16 tests? I'd expect it to be f32. bf16 is also fine but maybe not as the primary focus. |
@kuhar: we have bf16 tests with f32 accumulators, and we also have bf16 tests with bf16 accumulator. It's always explicit in the CMakeLists / BUILD.bazel, e.g.: this has iree/tests/e2e/matmul/CMakeLists.txt Line 486 in 5270093
The precision issues with large K sizes, which the commits in this PR address by relaxing tolerance, are of course only with bf16 accumulator. See how they are under Yes, f32 accumulator is what matters in practice. bf16 accumulator is something that we've supported in the CPU codegen mostly for completeness, so we also test it. |
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, this is starting to diverge from https://github.com/iree-org/iree-test-suites/tree/main/linalg_ops/matmul
I was hoping we could land iree-org/iree-test-suites#22 and switch from in-tree testing to using the test suite repo going forward, but that's been stalled. I can't devote much more time to that myself beyond the initial setup and prototyping (e.g. #18291)
This PR is made of individual commits for review convenience and so we can drop anything that causes problems on CI.