Skip to content

Conversation

@againull
Copy link
Contributor

No description provided.

@againull againull requested a review from a team as a code owner October 23, 2024 18:09
Comment on lines +43 to +44
assert(krnAttr.empty() ||
krnAttr == "intel_reqd_workgroup_walk_order(0,1,2)");
Copy link
Contributor

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

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

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.

@againull againull closed this Oct 30, 2024
@againull againull deleted the fix_attr_test branch September 29, 2025 21:30
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