-
Notifications
You must be signed in to change notification settings - Fork 769
[SYCL][E2E] Update some incompatible requirements #18007
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
Merged
Merged
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
b231bae
Update some incompatible requirements
KornevNikita e856d63
add run-aux for cpu aot runs
KornevNikita 6e27108
remove redundant stuff
KornevNikita f5791c8
upd
KornevNikita ad12314
fix enabled tests
KornevNikita 5890f7a
disable bad-context.cpp
KornevNikita 3a4c148
Merge remote-tracking branch 'upstream/sycl' into upd-some-e2e
KornevNikita 7163eb7
update bad-context.cpp
KornevNikita b254655
Merge remote-tracking branch 'upstream/sycl' into upd-some-e2e
KornevNikita File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
sycl/test-e2e/AddressSanitizer/invalid-argument/bad-context.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
According to other tests in this dir - I believe it's just a typo.
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.
No, this is not a typo, but maybe I used wrong pattern.
I want this test can be tested on machines which have both CPU and GPU, as you can see
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.
Okay, I got it, I'll update the REQUIRES string
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've updated this string - 7163eb7
Anyways, looks like this test wasn't launched in pre-commit on gen12 - it's in the list of unsupported tests.
@intel/dpcpp-devops-reviewers could you help me to figure out if it's true?
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.
yep, definitely looks like it didn't run. i dont think
cpu
andgpu
will ever both be true, i think we needany-device-is-cpu && any-device-is-gpu
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.
@sarnex take a look at this test - https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/AddressSanitizer/aot/cpu.cpp
the REQUIRES there is even simpler and it also uses run-aux, but it's also in the list of unsupported tests
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 ok yeah good find, let me actually debug it, thanks
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.
lol found the cause
https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/AddressSanitizer/lit.local.cfg#L16
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.
lol okay, then I think we can leave the test as is. it should work when the issue with gen12 is fixed.
@intel/dpcpp-sanitizers-review take a look please.
Uh oh!
There was an error while loading. Please reload this page.
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.
Thank you for correcting the require pattern.
Yep, we haven't fixed the GEN12 issue yet, because it's low priority.
We will take a look at GEN12 in the future.