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][SPIRV] Added a SPIR-V extension that introduces two new storage classes. #1840

Merged
merged 3 commits into from
Aug 26, 2020

Conversation

GarveyJoe
Copy link
Contributor

This extension defines two new storage classes: device and host that are subset of the cross workgroup storage class. The intention is to use them for USM pointers and non-USM accessors to provide additional information to the backends to enable optimization.

MrSidims added a commit to MrSidims/SPIRV-LLVM-Translator that referenced this pull request Jun 9, 2020
With this extension 2 new Storage Classes are introduced:
DeviceOnlyINTEL and HostOnlyINTEL appropriately mapped on
global_device and global_host SYCL/OpenCL address spaces which
are part of SYCL_INTEL_usm_address_spaces extension.

In case if device doesn't support SPV_INTEL_usm_storage_classes
extension global_device and global_host address spaces are lowered
to just global address space and therefore new Storage Classes are
lowered to CrossWorkgroup storage class.

Also this patch introduces a flag 'enable-usm-addrspaces' that
enables generation of global_device and global_host address spaces
during reversed translation.

Spec: intel/llvm#1840

Co-authored-by: Viktoria Maksimova <[email protected]>

Signed-off-by: Dmitry Sidorov <[email protected]>
Signed-off-by: Viktoria Maksimova <[email protected]>
@MrSidims
Copy link
Contributor

MrSidims commented Jun 9, 2020

There is an issue I faced during upstreaming the implementation in KhronosGroup/SPIRV-LLVM-Translator#579 . The issue is that spirv-val tool checks input/output parameters of GenericCastToPtr/PtrCastToGeneric instructions. And the current specification and therefore implementation violates the rules for these casts.

I can prepare a patch to the SPIR-V validation tool, but not sure if it will be accepted. Taking it into the account, does it make sense to remake casts by adding new instructions?

@MrSidims
Copy link
Contributor

MrSidims commented Jun 9, 2020

Taking it into the account, does it make sense to remake casts by adding new instructions?

It's not really blocking us, since I can just disable spirv-val tool in the testing, but it's kinda lame.

I can prepare a patch to the SPIR-V validation tool, but not sure if it will be accepted.

Well, actually, I can give it a try. The new rules shall be applied under:
if (_.HasExtension(SYCL_INTEL_usm_address_spaces)) {}

but for that we need to upstream the allocations from the extension to https://github.com/KhronosGroup/SPIRV-Headers first.

@bader bader added the spec extension All issues/PRs related to extensions specifications label Jun 11, 2020
@GarveyJoe
Copy link
Contributor Author

I changed the spec to add two new instructions for converting from device/host to crossworkgroup or vice versa instead of augmenting the existing conversion instructions.

…ddress spaces to SYCL to enable additional optimization.
@GarveyJoe
Copy link
Contributor Author

Added a SYCL extension to expose the SPIR-V extension.

@jbrodman jbrodman changed the title Added a SPIR-V extension that introduces two new storage classes. [SYCL][DOC][SPIRV] Added a SPIR-V extension that introduces two new storage classes. Jun 23, 2020
Modify Section 3.36.11, Conversion Instructions, adding two new instructions as follows:

|===
3+^| *OpPtrCastToCrossWorkgroupINTEL*
Copy link
Contributor

@AlexeySotkin AlexeySotkin Jun 25, 2020

Choose a reason for hiding this comment

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

Suggested change
3+^| *OpPtrCastToCrossWorkgroupINTEL*
4+| *OpPtrCastToCrossWorkgroupINTEL*

Comment on lines +114 to +115
_Result Type_ and _Pointer_ must point to the same type. 2+^| Capability: +
*USMStorageClassesINTEL*
Copy link
Contributor

@AlexeySotkin AlexeySotkin Jun 25, 2020

Choose a reason for hiding this comment

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

Suggested change
_Result Type_ and _Pointer_ must point to the same type. 2+^| Capability: +
*USMStorageClassesINTEL*
_Result Type_ and _Pointer_ must point to the same type. 1+| Capability: +
*USMStorageClassesINTEL*

|===

|===
3+^| *OpCrossWorkgroupCastToPtrINTEL*
Copy link
Contributor

@AlexeySotkin AlexeySotkin Jun 25, 2020

Choose a reason for hiding this comment

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

Suggested change
3+^| *OpCrossWorkgroupCastToPtrINTEL*
4+| *OpCrossWorkgroupCastToPtrINTEL*

Comment on lines +130 to +131
_Result Type_ and _Pointer_ must point to the same type. 2+^| Capability: +
*USMStorageClassesINTEL*
Copy link
Contributor

@AlexeySotkin AlexeySotkin Jun 25, 2020

Choose a reason for hiding this comment

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

Suggested change
_Result Type_ and _Pointer_ must point to the same type. 2+^| Capability: +
*USMStorageClassesINTEL*
_Result Type_ and _Pointer_ must point to the same type. 1+| Capability: +
*USMStorageClassesINTEL*

MrSidims added a commit to MrSidims/llvm that referenced this pull request Jul 7, 2020
With this patch an accessor pointer to global buffer is moved from
global space to global_device space. That is done to distinguish this
pointer from those USM pointers, that are allocated global space or
global_host space, in compile time.

In addition to this change there are added explicit conversion operator
from global_device to global space for multi_ptr class and implicit
convertion for atomic class from global_device for global space.
The last change isn't covered by specification published here:
intel#1840 , but is required to pass
atomic_api CTS.

Signed-off-by: Dmitry Sidorov <[email protected]>
bader pushed a commit that referenced this pull request Jul 10, 2020
With this patch an accessor pointer to global buffer is moved from
global space to global_device space. That is done to distinguish this
pointer from those USM pointers, that are allocated global space or
global_host space, in compile time.

In addition to this change there are added explicit conversion operator
from global_device to global space for multi_ptr class and implicit
convertion for atomic class from global_device for global space.
The last change isn't covered by specification published here:
#1840 , but is required to pass
atomic_api CTS.

Signed-off-by: Dmitry Sidorov <[email protected]>
@romanovvlad
Copy link
Contributor

@AlexeySotkin @MrSidims Could you please approve if the change is OK with you?

Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

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

SPIR-V part is LGTM

@romanovvlad romanovvlad merged commit 781fbfc into sycl Aug 26, 2020
@bader bader deleted the GarveyJoe-patch-1 branch September 4, 2020 17:12
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.

6 participants