Skip to content

[SYCL][E2E] Fix Basic/kernel_info_attr.cpp test #15837

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

Closed
wants to merge 1 commit into from
Closed

[SYCL][E2E] Fix Basic/kernel_info_attr.cpp test #15837

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 13 additions & 6 deletions sycl/test-e2e/Basic/kernel_info_attr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out
//
// Fail is flaky for level_zero, enable when fixed.
// Fail is flaky for level_zero caused by issue in UR Level Zero adapter:
// URLZA-419
// UNSUPPORTED: level_zero
//
// Consistently fails with opencl gpu, enable when fixed.
// XFAIL: opencl && gpu
// XFAIL-TRACKER: GSD-8971

//==--- kernel_info_attr.cpp - SYCL info::kernel::attributes test ---==//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
Expand Down Expand Up @@ -37,7 +34,17 @@ int main() {
});

const std::string krnAttr = krn.get_info<info::kernel::attributes>();
assert(krnAttr.empty());
if (q.get_device().get_platform().get_info<info::platform::vendor>() ==
"Intel(R) Corporation" &&
q.get_device().get_info<info::device::device_type>() ==
info::device_type::gpu) {
// Older intel drivers don't attach any default attributes and newer ones
// force walk order to X/Y/Z using special attribute.
assert(krnAttr.empty() ||
krnAttr == "intel_reqd_workgroup_walk_order(0,1,2)");
Comment on lines +43 to +44
Copy link
Contributor

@aelovikov-intel aelovikov-intel Oct 23, 2024

Choose a reason for hiding this comment

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

@gmlueck is this legal behavior for the implementation to attach some attributes implicitly?

Copy link
Contributor

@gmlueck gmlueck Oct 30, 2024

Choose a reason for hiding this comment

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

I agree that it's weird, especially since intel_reqd_workgroup_walk_order isn't documented. It seems like intel_reqd_workgroup_walk_order is more of an implementation detail, rather than a user-visible attribute. Given that, it seems like it might be better to not return it from get_info<info::kernel::attributes>(). Would that be easy to do?

On the other hand, the SYCL spec doesn't define the format of the string returned from get_info<info::kernel::attributes>(), so the current behavior won't cause us to fail conformance.

Copy link
Contributor Author

@againull againull Oct 30, 2024

Choose a reason for hiding this comment

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

I see your point. It seems that this at least breaks conformance with OpenCL spec as it says the following about CL_KERNEL_ATTRIBUTES:

Returns any attributes specified using the attribute OpenCL C qualifier (or using an OpenCL C++ qualifier syntax [[]] ) with the kernel function declaration in the program source.

Currently gpu driver returns intel_reqd_workgroup_walk_order even though nothing is attached.
So, problem should be fixed at gpu driver level to not return internal attributes for corresponding queries.

} else {
assert(krnAttr.empty());
}
const std::string krnAttrExt =
syclex::get_kernel_info<SingleTask, info::kernel::attributes>(ctx);
assert(krnAttr == krnAttrExt);
Expand Down
Loading