-
Notifications
You must be signed in to change notification settings - Fork 797
[SYCL][Tests] Add more syclbin-dump tests #19272
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
@@ -50,19 +50,6 @@ raw_ostream &operator<<(raw_ostream &OS, const ScopedIndent &) { | |||
return OS.indent(CurrentIndentationLevel); | |||
} | |||
|
|||
std::string_view StateToString(llvm::object::SYCLBIN::BundleState State) { |
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.
Removed as it was not used and it was affecting the code coverage
sycl/test/syclbin/malformed.syclbin
Outdated
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.
Should we move this to an Inputs subfolder?
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.
Overall I think this looks good!
// CHECK-EXECUTABLE-BMG-G21-GPU-DEVICE-ARCH-NEXT: SYCLBIN/native device code image metadata: | ||
// CHECK-EXECUTABLE-BMG-G21-GPU-DEVICE-ARCH-NEXT: arch:{{.*}} | ||
// CHECK-EXECUTABLE-BMG-G21-GPU-DEVICE-ARCH-NEXT: target:{{.*}}spir64_gen-unknown | ||
// CHECK-EXECUTABLE-BMG-G21-GPU-DEVICE-ARCH-NEXT: Raw native device code image bytes: <Binary blob of {{.*}} bytes> |
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 think it would be fine to have this in sycl/test instead.
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.
Let's try, should work. Still investigating though why it doesn't work in CI - as it works locally. The binary now in the install folder, but still it didn't fix the issue, probably need to add to some other place in CMake files.
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.
The only reason I see not putting it to sycl/test folder is that the test has a dependency on NEO - especially on ocloc
binary, and sycl/test tests should be device-agnostic (no backend runtimes should be present in the environment)
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.
Hmm, good point. Should it have //REQUIRES: ocloc
?
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.
Let's try, should work. Still investigating though why it doesn't work in CI - as it works locally. The binary now in the install folder, but still it didn't fix the issue, probably need to add to some other place in CMake files.
Maybe we need to do something like in https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/lit.cfg.py#L627?
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.
Let's try, should work. Still investigating though why it doesn't work in CI - as it works locally. The binary now in the install folder, but still it didn't fix the issue, probably need to add to some other place in CMake files.
Maybe we need to do something like in https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/lit.cfg.py#L627?
Thanks! It works now :)
Failed Tests (1): |
@dm-vodopyanov New failures in postcommit:
Can you please fix ASAP or revert? Thx |
@dm-vodopyanov Can you please comment on your plans to address the postcommit failures? Otherwise I will revert this PR FYI @intel/llvm-gatekeepers |
Should we disable the test for now, and @dm-vodopyanov can re-enable it after fixing? |
Sure, will do |
No description provided.