Skip to content

[SYCL][ABI] Subgroup Extension spec update #1600

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 3 commits into from
May 15, 2020

Conversation

garimagu
Copy link
Contributor

@garimagu garimagu commented Apr 28, 2020

Implements the change as per the spec update at
https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/SubGroup/SYCL_INTEL_sub_group.asciidoc
and update from : #1565

ABI update: Symbols corresponding to the deleted APIs have been deleted.

Signed-off-by: Garima Gupta garima.gupta@intel.com

if (ret_err != CL_SUCCESS)
return cast<pi_result>(ret_err);

*((uint32_t *)param_value) = (uint32_t)ret_val;
Copy link
Contributor

Choose a reason for hiding this comment

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

the OpenCL returns "size_t", why are you truncating this to 32-bit? If really needed (I don't think it is) we should document this for piKernelGetSubGroupInfo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The return type of the get_sub_group_info APIs have been changed to return uint32_t data instead of size_t, which is why this change was done. How should this be documented?

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment in the pi.h for piKernelGetSubGroupInfo to indicated now supported parameters with their sizes.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I suggest that we continue to use "size_t" in PI, and truncate at SYCL level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is that?

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 have made the call to PI_API check the returned size of the result value and truncate the value appropriately. Does this look better?
I'm essentially hesitant to make it size_t based on OpenCL spec, because if any new backend is developed based on the SYCL extension spec, it will have to change the Plugin to cast the uint32_t to size_t first, and then in the library will be casting the size_t to uint32_t. It will be weird that the PI APIs are not SYCL extension spec compliant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm essentially hesitant to make it size_t based on OpenCL spec

I'd be fine making it uint32_t in PI, document this in the pi.h, and truncate in OpenCL plugin.
But we should stick to just one size that fits SYCL well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smaslov-intel : Do you mean the initial solution was ok with the document change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I'd be fine with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I have updated the review. Let me know if it looks good.

@alexbatashev
Copy link
Contributor

@garimagu your patch very likely conflicts with recently merged #1609. Please, update your branch.

@garimagu garimagu force-pushed the private/garimagu/subgroup_extension branch from 102c7eb to 11bcbd9 Compare April 30, 2020 19:55
@garimagu garimagu requested a review from smaslov-intel May 5, 2020 19:04
@garimagu
Copy link
Contributor Author

garimagu commented May 5, 2020

@smaslov-intel Ping.

@garimagu garimagu force-pushed the private/garimagu/subgroup_extension branch from eac4935 to ac04c3f Compare May 6, 2020 22:13
Copy link
Contributor

@alexbatashev alexbatashev left a comment

Choose a reason for hiding this comment

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

@garimagu since this PR removes two functions, you also need to uplift DEV_ABI version

set(SYCL_DEV_ABI_VERSION 1)
according to ABI Policy Guide.

@garimagu
Copy link
Contributor Author

garimagu commented May 7, 2020

@garimagu since this PR removes two functions, you also need to uplift DEV_ABI version

set(SYCL_DEV_ABI_VERSION 1)

according to ABI Policy Guide.

What should the new SYCL_DEV_ABI_VERSION be?
Is there any document for ABI Policy Guide?

@garimagu garimagu force-pushed the private/garimagu/subgroup_extension branch from ac04c3f to 602c90f Compare May 7, 2020 19:07
smaslov-intel
smaslov-intel previously approved these changes May 7, 2020
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

LGTM

@alexbatashev
Copy link
Contributor

@garimagu since this PR removes two functions, you also need to uplift DEV_ABI version

set(SYCL_DEV_ABI_VERSION 1)

according to ABI Policy Guide.

What should the new SYCL_DEV_ABI_VERSION be?
Is there any document for ABI Policy Guide?

https://github.com/intel/llvm/blob/sycl/sycl/doc/ABIPolicyGuide.md
The current value is 1, so it must become 2.

@garimagu garimagu force-pushed the private/garimagu/subgroup_extension branch from e083664 to b4c6d7b Compare May 11, 2020 17:05
@garimagu
Copy link
Contributor Author

@garimagu since this PR removes two functions, you also need to uplift DEV_ABI version

set(SYCL_DEV_ABI_VERSION 1)

according to ABI Policy Guide.

What should the new SYCL_DEV_ABI_VERSION be?
Is there any document for ABI Policy Guide?

https://github.com/intel/llvm/blob/sycl/sycl/doc/ABIPolicyGuide.md
The current value is 1, so it must become 2.

Thank you for the link @alexbatashev . Let me know if I have missed something. I have updated the Major version, the ABI version and the PR commit message.

@garimagu garimagu changed the title [SYCL] Subgroup Extension spec update [SYCL][ABI] Subgroup Extension spec update May 11, 2020
@@ -8,10 +8,10 @@ set(CMAKE_CXX_EXTENSIONS OFF)
option(SYCL_ENABLE_WERROR "Treat all warnings as errors in SYCL project" OFF)
option(SYCL_ADD_DEV_VERSION_POSTFIX "Adds -V postfix to version string" ON)

set(SYCL_MAJOR_VERSION 0)
set(SYCL_MAJOR_VERSION 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

@garimagu the major version is updated once per release. You only need to update DEV_ABI version.

Suggested change
set(SYCL_MAJOR_VERSION 1)
set(SYCL_MAJOR_VERSION 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uploaded the change. please review and approve. smaslov-intel has approved the changes other than the ABI version update.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should (semi-)automate SYCL version update.
It's hard to manage multiple commits requiring version update.

@alexbatashev, could you create a post-commit job, which will create a PR with the version update if it's required, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bader I'll take a look into it

smaslov-intel
smaslov-intel previously approved these changes May 11, 2020
alexbatashev
alexbatashev previously approved these changes May 11, 2020
Implements the change as per the spec update at
https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/SubGroup/SYCL_INTEL_sub_group.asciidoc
and update from : intel#1565

ABI update: Functions corresponding to the deleted APIs have been
deleted.
Edit info.cpp test to expect max sub-group size based on the spec update.
Changed pi.h document and changed the plugin to always return a 32 bit
value.
ABI version update.

Signed-off-by: Garima Gupta <garima.gupta@intel.com>
@garimagu garimagu dismissed stale reviews from alexbatashev and smaslov-intel via 21c9f6f May 12, 2020 16:54
@garimagu garimagu force-pushed the private/garimagu/subgroup_extension branch from cee31e8 to 21c9f6f Compare May 12, 2020 16:54
Signed-off-by: Garima Gupta <garima.gupta@intel.com>
@garimagu
Copy link
Contributor Author

@bader Please approve and merge.
Sergey has approved it before the final small fix of pi_opencl_symbol_check.dump

@bader
Copy link
Contributor

bader commented May 13, 2020

Waiting on code owner review from smaslov-intel and/or intel/llvm-reviewers-runtime.

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

LGTM. A few minor suggestions.

template <info::kernel_sub_group Param> struct get_kernel_sub_group_info {
static uint32_t get(RT::PiKernel Kernel, RT::PiDevice Device,
const plugin &Plugin) {
uint32_t Result;
// TODO catch an exception and put it to list of asynchronous exceptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// TODO catch an exception and put it to list of asynchronous exceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this comment be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, this comment is not valid anymore. Most of the errors now reported through synchronous exceptions.
Although, I'm not the owner of the runtime and let @intel/llvm-reviewers-runtime to decide.

smaslov-intel
smaslov-intel previously approved these changes May 13, 2020
Signed-off-by: Garima Gupta <garima.gupta@intel.com>
@garimagu
Copy link
Contributor Author

@bader uploaded the changes. smaslov has also approved the changes just before the suggested changes were merged.

@garimagu
Copy link
Contributor Author

@bader ping

@bader bader requested a review from smaslov-intel May 15, 2020 17:00
@bader bader merged commit 9d4c284 into intel:sycl May 15, 2020
kbenzie added a commit to kbenzie/intel-llvm that referenced this pull request Feb 17, 2025
…ctoring

[L0] Refactoring of boolean event parameters
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
[L0] Refactoring of boolean event parameters
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.

5 participants