Skip to content
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

[SYCL][Doc] Clarify InvokeSIMD + SYCL_EXTERNAL #5353

Merged
merged 2 commits into from Jan 27, 2022

Conversation

Pennycook
Copy link
Contributor

The interaction between the InvokeSIMD extension and SYCL_EXTERNAL
functions was previously undefined. This commit clarifies when such
interactions are expected to fail and why.

Signed-off-by: John Pennycook john.pennycook@intel.com

The interaction between the InvokeSIMD extension and SYCL_EXTERNAL
functions was previously undefined. This commit clarifies when such
interactions are expected to fail and why.

Signed-off-by: John Pennycook <john.pennycook@intel.com>
@Pennycook Pennycook added the spec extension All issues/PRs related to extensions specifications label Jan 20, 2022
@Pennycook Pennycook requested a review from a team as a code owner January 20, 2022 19:38
This is a preview extension specification, intended to provide early access to a feature for review and community feedback. When the feature matures, this specification may be released as a formal extension.

Because the interfaces defined by this specification are not final and are subject to change they are not intended to be used by shipping software products.
This is a proposed extension specification, intended to gather community

Choose a reason for hiding this comment

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

This is just a formatting comment, but does asciidoc care about newlines in paragraphs? I ask because it seems like the previous version had long lines, while this change is re-flowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

asciidoc doesn't, but other reviewers have complained about long lines in extension documentation. Some of the boilerplate content from older specifications still have long lines; I saw this as an opportunity to update InvokeSIMD to use some of the newer boilerplate content (with short lines).

Copy link

@jasonsewall-intel jasonsewall-intel left a comment

Choose a reason for hiding this comment

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

I think this is a bit of a punt in the sense that we don't really solve the problem of wanting to call a properly compiled SYCL_EXTERNAL function, but I think it is fine for the time being. It would be good to think about the linkage issue and ways to control how external functions are compiled down the road.

Signed-off-by: John Pennycook <john.pennycook@intel.com>
@Pennycook
Copy link
Contributor Author

I think this is a bit of a punt in the sense that we don't really solve the problem of wanting to call a properly compiled SYCL_EXTERNAL function, but I think it is fine for the time being. It would be good to think about the linkage issue and ways to control how external functions are compiled down the road.

You're right, but I think we need more implementation experience first.

I suspect that the upcoming ESIMD-based implementation of InvokeSIMD is going to require an ESIMD-specific macro, and in the short-term we'll probably also need some CUDA-backend-specific control for overriding the default compiler behavior when targeting PTX.

Long-term I hope we can adopt something portable and similar to the function properties design described in the latest revision of the KernelProperties extension (see here):

SYCL_EXT_ONEAPI_FUNCTION_PROPERTY((sycl::simd_property_name_tbd))
sycl::simd<float, 8> simd_function(sycl::simd<float, 8> x);

...but we'll need KernelProperties to be implemented first, and to figure out exactly what the property describes. Some backends may be satisfied just by "this is for SIMD", but I'm worried that some will need more information (e.g. the sub-group size of the kernel expected to call the SIMD function, maybe?).

@Pennycook
Copy link
Contributor Author

@bader I think this one is fine to merge; @rolandschulz did review 7 days ago, and has marked the only issue raised as resolved.

@bader bader merged commit ac3e816 into intel:sycl Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec extension All issues/PRs related to extensions specifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants