-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL][E2E] Ensure lowering of llvm.bitreverse for 2/4-bit scalars is functionally correct #13359
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
Conversation
@LU-JOHN Introduced test fails in pre-commit:
|
@@ -2,6 +2,10 @@ | |||
|
|||
// UNSUPPORTED: hip || cuda | |||
|
|||
// TODO: Remove XFAIL after fixing | |||
// https://github.com/intel/intel-graphics-compiler/issues/330 | |||
// XFAIL: gpu-intel-gen12 |
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.
Is this test passing on other GPUs (like pvc, dg2)?
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.
On commit 1ad567a3f7f47920549bb58d6b433b7af869c7dd, it was only failing on:
SYCL Pre Commit on Windows / e2e / Intel GEN12 Graphics with Level Zero
SYCL Pre Commit on Linux / test (Intel, ["Linux", "gen12"], ghcr.io/intel/llvm/ubuntu2204_intel_drivers
Everything is passing now with the latest commit d7b722ec0d1eaa22dbfaceb17d1525f4418738f5.
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.
We test only on Gen12 in pre-commit testing, so because you marked the test with XFAIL: gpu-intel-gen12, it passes now on Gen12. Unfortunately, it doesn't mean that test passes on other hardware. So it may easily fail on DG2 testing in post-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.
Also, does it make sense to land this PR since IGC has some issues with these instructions?
Basically, because of this you disable testing of all other builtins (for other types).
Maybe it worth outlining this builtings to a separate test? And maybe even check only llvm-spirv translation for now (not e2e)?
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.
Test has a segmentation fault on pvc machine and dg2 machine.
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.
2/4-bit bitreverse testing moved to new file.
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.
Marking as requested changes for now to avoid accidental merge, just because some issues need to be addressed first.
… functionally correct Signed-off-by: Lu, John <[email protected]>
Signed-off-by: Lu, John <[email protected]>
Signed-off-by: Lu, John <[email protected]>
Signed-off-by: Lu, John <[email protected]>
Signed-off-by: Lu, John <[email protected]>
Signed-off-by: Lu, John <[email protected]>
Post-commit failure should be addressed by #13889. |
Ensure that lowering of llvm.bitreverse.i2/i4 by llvm-spirv is functionally correct.