Skip to content

Conversation

@lbushi25
Copy link
Contributor

@lbushi25 lbushi25 commented Sep 23, 2024

Removed the XFAIL directive from several joint matrix tests as they are now passing on all devices except for DG2 GPU because apparently it does not yet support sub-group size of 32 for joint matrix and it also does not support VNNI transforms. Hence, these tests are marked as UNSUPPORTED on DG2 and on all other devices they are now expected to pass.

@lbushi25 lbushi25 changed the title [SYCL] Reenable Matrix E2E tests [SYCL] Reenable several joint matrix E2E tests Sep 23, 2024
@lbushi25 lbushi25 marked this pull request as ready for review September 23, 2024 17:49
@lbushi25 lbushi25 requested a review from a team as a code owner September 23, 2024 17:49
@lbushi25 lbushi25 changed the title [SYCL] Reenable several joint matrix E2E tests [SYCL] Re-enable several joint matrix E2E tests Sep 23, 2024
// RUN: %{run} %t.out

// Currently row major B fails when annotated_ptr is used
// XFAIL: gpu
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this test pass on PVC? I did not see any unexpected pass on PVC for this test...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to test this on PVC. I just tested it now and yes you are right, it fails.
I'm adding an unsupported directive for pvc on these tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

The test is supported on PVC but it is XFAIL.
So you should keep both:
// UNSUPPORTED: gpu-intel-dg2
and
// XFAIL: gpu
(same goes to the other tests)

// This tests support of col major layout for matrix B which does transpose and
// then VNNI transform. This is currently only available on AMX

// XFAIL: gpu
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this test pass on PVC? I did not see any unexpected pass on PVC for this test...

// RUN: %{build} -o %t.out
// RUN: %{run} %t.out

// XFAIL:*
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this test pass on PVC? I did not see any unexpected pass on PVC for this test...

// This tests support of col major layout for matrix B which does transpose and
// then VNNI transform. This is currently only available on AMX

// XFAIL: gpu
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this test pass on PVC? I did not see any unexpected pass on PVC for this test...

// This tests support of col major layout for matrix B which does transpose and
// then VNNI transform. This is currently only available on AMX

// XFAIL: gpu
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this test pass on PVC? I did not see any unexpected pass on PVC for this test...

// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
// VNNI transform and sub-group size 32 are not supported yet on DG2 by IGC
Copy link
Contributor

@YuriPlyakhin YuriPlyakhin Sep 23, 2024

Choose a reason for hiding this comment

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

sub-group size 32 part is not relevant for this test. To avoid confusion, I suggest mention only VNNI transform is not supported.

Edit: also, after looking at implementation I see that some VNNI transform is implemented on DG2 in IGC, so in general this statement is not correct. It needs to be more specific. Same is true for other messages below, which mention VNNI transform is not supported.

// RUN: %{build} -o %t.out
// RUN: %{run} %t.out

// XFAIL:*
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this test pass on PVC? I did not see any unexpected pass on PVC for this test...

// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
// VNNI transform and sub-group size 32 are not supported yet on DG2 by IGC
Copy link
Contributor

@YuriPlyakhin YuriPlyakhin Sep 23, 2024

Choose a reason for hiding this comment

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

sub-group size 32 part is not relevant for this test. VNNI transform part of the statement needs to be more specific.

// This tests support of col major layout for matrix B which does transpose and
// then VNNI transform. This is currently only available on AMX

// XFAIL: gpu
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this test pass on PVC? I did not see any unexpected pass on PVC for this test...

Copy link
Contributor

@YuriPlyakhin YuriPlyakhin left a comment

Choose a reason for hiding this comment

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

I think this change should not be committed. Please, check my comments. Thanks.

// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
// VNNI transform and sub-group size 32 are not supported yet on DG2 by IGC
Copy link
Contributor

@YuriPlyakhin YuriPlyakhin Sep 23, 2024

Choose a reason for hiding this comment

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

sub-group size 32 part is not relevant for this test. VNNI transform part needs to be more specific

@lbushi25
Copy link
Contributor Author

I think this change should not be committed. Please, check my comments. Thanks.

Ok, closing this PR. Instead, I'm creating an issue to track the fail on PVC and later add a reference to that issue as a comment in the test.

@lbushi25 lbushi25 closed this Sep 23, 2024
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.

3 participants