Skip to content

[SYCL][Joint Matrix][E2E] Uncomment the Joint Matrix tests for combination 32x32x16 #16191

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

Merged
merged 4 commits into from
Dec 3, 2024
Merged

[SYCL][Joint Matrix][E2E] Uncomment the Joint Matrix tests for combination 32x32x16 #16191

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf10daf
Uncomment the tests for Joint Matrix combination 32x32x16
YixingZhang007 Nov 26, 2024
7561b2f
resolve comments
YixingZhang007 Dec 2, 2024
f764365
code clean up
YixingZhang007 Dec 2, 2024
48957e1
code clean up
YixingZhang007 Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions sycl/test-e2e/Matrix/element_wise_all_ops_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,9 @@ int main() {
test_ewops_ab<bfloat16, 8, 16, use::a, layout::row_major, 1>();
test_ewops_ab<bfloat16, 16, 8, use::b, layout::ext_intel_packed, 2>();
test_ewops_c<float, 8, 8>();
// test_ewops_ab<bfloat16, 32, 16, use::a, layout::row_major, 1>();
// test_ewops_ab<bfloat16, 16, 32, use::b, layout::ext_intel_packed, 2>();
// test_ewops_c<float, 32, 32>();
test_ewops_ab<bfloat16, 32, 16, use::a, layout::row_major, 1>();
test_ewops_ab<bfloat16, 16, 32, use::b, layout::ext_intel_packed, 2>();
test_ewops_c<float, 32, 32>();
break;
}
}
Expand Down
3 changes: 1 addition & 2 deletions sycl/test-e2e/Matrix/element_wise_ops_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,7 @@ int main() {
passed &= test<uint8_t, int32_t, 8, 8, 32, 4, class dg2_uint_8x8x32>();
passed &= test<int8_t, int32_t, 8, 8, 32, 4, class dg2_sint_8x8x32>();
passed &= test<bfloat16, float, 8, 8, 16, 2, class dg2_bf16_8x16x16>();
// passed &= test<bfloat16, float, 32, 32, 16, 2, class
// dg2_bf16_32x32x16>();
passed &= test<bfloat16, float, 32, 32, 16, 2, class dg2_bf16_32x32x16>();
break;
}
}
Expand Down
2 changes: 2 additions & 0 deletions sycl/test-e2e/Matrix/joint_matrix_bf16_fill_k_cache_arg_dim.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,7 @@

// -ffp-model=precise is added to not depend on compiler defaults.

// XFAIL: gpu
Copy link
Contributor

@dkhaldi dkhaldi Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add
// XFAIL-TRACKER: CMPLRLLVM-63710
so you don't increase number of xfail without tracker

Also, should you add the xfail on only !igc-dev?

Copy link
Contributor Author

@YixingZhang007 YixingZhang007 Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! I have made the change.
These two tests are not supported in the igc-dev CI test because the commit in IGC has not yet been integrated into the driver. I will remove the XFAIL once the IGC change has been pull downed.

Copy link
Contributor

@YuriPlyakhin YuriPlyakhin Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YixingZhang007 ,
looking at the branch that is used for igc-dev here https://github.com/intel/llvm/blob/sycl/devops/dependencies-igc-dev.json

it should include your changes for runtime dimension support:
https://github.com/intel/intel-graphics-compiler/commits/6ee988a5948c6aaecbfe00605f52694c55cf1052/IGC/Compiler/Optimizer/OpenCLPasses/JointMatrixFuncsResolutionPass/JointMatrixFuncsResolutionPass.cpp

So, tests should be passing already with igc-dev in intel/llvm CI.
What am I missing?

Copy link
Contributor Author

@YixingZhang007 YixingZhang007 Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve noticed that the igc_dev CI test is not reflecting the IGC commits it's supposed to be testing. It seems to still be using an older version of IGC, likely the version set for the regular IGC.

I’m experiencing the same issue with another LLVM PR (#15880), where my IGC commit should already be integrated into igc_dev, but the tests get_coordinate_ops.cpp is still failing due to errors that should have been addressed by that IGC commit. As a temporary approach, I’ve added XFAIL to the test while we wait for this issue to be resolved or until the regular IGC is updated to include the changes.

Please let me know if this approach would work and if you have any suggestions! Thanks!

Copy link
Contributor

@YuriPlyakhin YuriPlyakhin Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YixingZhang007 ,
"while we wait for this issue to be resolved" - does it mean someone is working on resolving the issue? If not, could you please submit the issue to https://github.com/intel/llvm/issues, so that it is reported, and we can assign someone to work on it?
@jsji , @sarnex , FYI

I'm OK with adding XFAIL, since we have this problem.

Copy link
Contributor

@sarnex sarnex Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are working on something called IGC 2 I think, so maybe they enabled it by default?

Copy link
Contributor

@sarnex sarnex Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intel/intel-graphics-compiler@4617a3e seems intended to me

Copy link
Contributor Author

@YixingZhang007 YixingZhang007 Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created an issue #16251 regarding this.

Copy link
Contributor

@jsji jsji Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, we need to make sure we softlink to libigc.so to libigc.so.2 for igc-dev now.

Copy link
Contributor

@sarnex sarnex Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah im gonna look at the issue and try to see what's going on, i gotta fix igc-dev containers first


#include "common.hpp"
#include "joint_matrix_bf16_fill_k_cache_impl.hpp"
4 changes: 2 additions & 2 deletions sycl/test-e2e/Matrix/joint_matrix_bf16_fill_k_cache_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,8 +493,8 @@ size_t matrix_size = -1;

test<bfloat16, float, VnniFactor, /*TM*/ 8, /*TN*/ 8, /*TK*/ 16, MCache1,
NCache1, KCache1, MCache2, NCache2, KCache2>(matrix_size);
// test<bfloat16, float, VnniFactor, /*TM*/ 32, /*TN*/ 32, /*TK*/ 16, MCache1,
// NCache1, KCache1, MCache2, NCache2, KCache2>(matrix_size);
test<bfloat16, float, VnniFactor, /*TM*/ 32, /*TN*/ 32, /*TK*/ 16,
MCache1, NCache1, KCache1, MCache2, NCache2, KCache2>(matrix_size);
break;
}
}
Expand Down
2 changes: 2 additions & 0 deletions sycl/test-e2e/Matrix/joint_matrix_bf16_fill_k_cache_runtime_dim.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,7 @@

// -ffp-model=precise is added to not depend on compiler defaults.

// XFAIL: gpu

Copy link
Contributor

@dkhaldi dkhaldi Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add
// XFAIL-TRACKER: CMPLRLLVM-63710
so you don't increase number of xfail without tracker

Copy link
Contributor Author

@YixingZhang007 YixingZhang007 Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I have updated the test.

#include "common.hpp"
#include "joint_matrix_bf16_fill_k_cache_impl.hpp"
4 changes: 2 additions & 2 deletions sycl/test-e2e/Matrix/joint_matrix_rowmajorA_rowmajorB_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,8 @@ int main() {
gemm_row_major<8, 8, 32, class su_8x8x32, int8_t, uint8_t, int32_t>();
res += gemm_row_major<8, 8, 32, class uu_8x8x32, uint8_t, uint8_t,
int32_t>();
// res += gemm_row_major<32, 32, 16, class dg2_bf16_32x32x16, bfloat16,
// bfloat16, float>();
res += gemm_row_major<32, 32, 16, class dg2_bf16_32x32x16, bfloat16,
bfloat16, float>();
break;
}
}
Expand Down
4 changes: 3 additions & 1 deletion sycl/test/e2e_test_requirements/no-xfail-without-tracker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
// tests to match the required format and in that case you should just update
// (i.e. reduce) the number and the list below.
//
// NUMBER-OF-XFAIL-WITHOUT-TRACKER: 77
// NUMBER-OF-XFAIL-WITHOUT-TRACKER: 79
Copy link
Contributor

@maarquitos14 maarquitos14 Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quoting from line 43 from this very same file:

// That number *must not* increase. Any PR which causes this number to grow
// should be rejected and it should be updated to either keep the number as-is
// or have it reduced (preferably, down to zero).

Please, read carefully the header of this file to understand what you need to do. Basically, you cannot add new XFAIL directives without a tracker to address it.

Copy link
Contributor Author

@YixingZhang007 YixingZhang007 Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the mistake! Thanks for pointing it out :)
I have removed the change in file no-xfail-without-tracker.cpp, and added XFAIL-TRACKER to the tests.

//
// List of improperly XFAIL-ed tests.
// Remove the CHECK once the test has been properly XFAIL-ed.
Expand Down Expand Up @@ -96,6 +96,8 @@
// CHECK-NEXT: Matrix/SG32/joint_matrix_prefetch.cpp
// CHECK-NEXT: Matrix/SG32/joint_matrix_rowmajorA_rowmajorB.cpp
// CHECK-NEXT: Matrix/SG32/joint_matrix_unaligned_k.cpp
// CHECK-NEXT: Matrix/joint_matrix_bf16_fill_k_cache_arg_dim.cpp
// CHECK-NEXT: Matrix/joint_matrix_bf16_fill_k_cache_runtime_dim.cpp
Copy link
Contributor

@dkhaldi dkhaldi Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these two addition as you add the trackers in the tests

// CHECK-NEXT: Matrix/joint_matrix_bfloat16_colmajorA_colmajorB.cpp
// CHECK-NEXT: Matrix/joint_matrix_colA_rowB_colC.cpp
// CHECK-NEXT: Matrix/joint_matrix_int8_colmajorA_colmajorB.cpp
Expand Down