Add support for SPV_INTEL_usm_storage_classes extension #579

MrSidims · 2020-06-09T08:42:26Z

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.

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 · 2020-06-09T08:42:51Z

Tagging @vmaksimo @mlychkov @AlexeySotkin @AlexeySachkov

MrSidims · 2020-06-09T11:16:03Z

The error:

error: line 138: Expected input to have storage class Generic: GenericCastToPtr
%59 = OpGenericCastToPtr %_ptr_CrossWorkgroup_uint %58

which I don't really understand from where it comes. Would appreciate any kind of hint.

vmaksimo · 2020-06-09T13:26:47Z

The error:

error: line 138: Expected input to have storage class Generic: GenericCastToPtr
%59 = OpGenericCastToPtr %_ptr_CrossWorkgroup_uint %58

which I don't really understand from where it comes. Would appreciate any kind of hint.

@MrSidims This error is produced by spirv-val. It doesn't fail locally if you don't have it in the PATH variable

MrSidims · 2020-06-09T14:57:26Z

The error:

error: line 138: Expected input to have storage class Generic: GenericCastToPtr
%59 = OpGenericCastToPtr %_ptr_CrossWorkgroup_uint %58

which I don't really understand from where it comes. Would appreciate any kind of hint.

@MrSidims This error is produced by spirv-val. It doesn't fail locally if you don't have it in the PATH variable

Got it, thanks! The reasons is that spirv-val tools checks that GenericCastToPtr has input of Generic storage type, while in the extension we allows an input of CrossWorkgroup storage class in case if it's being casted to Device/Host.

MrSidims · 2020-06-09T15:15:01Z

I have disabled spirv-val for now. But may be we will reconsider the specification.

MrSidims · 2020-06-09T16:00:02Z

@svenvh @AlexeySotkin can I ask your opinion of changing rules for already existing instructions GenericCastToPtr and PtrCastToGeneric (under the extension) and if it's possible to upstream the changes to spirv-val tool? If it's not OK we can just implement new instructions to reflect desired address space casts in SPIR-V.

AlexeySachkov

A few question prior proper detailed review

AlexeySachkov · 2020-06-09T17:43:06Z

lib/SPIRV/SPIRVReader.cpp

+        SPIRSPIRVAddrSpaceMap::rmap(T->getPointerStorageClass());
+    // If generation of SYCL USM address spaces isn't allowed in the reversed
+    // translation - transform them into global address space
+    if (!BM->isUsmAddrspacesEnabled() &&


What is the reason to add this option? I mean, new storage classes enable USMStorageClassesINTEL capability in the module and it is responsibility of the particular consumer to either reject SPIR-V with such capability as unsupported or accept new storage classes, which can be done by lowering them to whichever address spaces or to anything else what is desired by the consumer.

So, the generator is responsible to either generate more portable SPIR-V, which doesn't use the extension, or generate less portable SPIR-V, which uses the extension, but cannot be supplied to any consumer (because of lack of the extension support).

If consumer is aware of that option, it means that it is aware of that extension and therefore, it could perform desired lowering by itself and code in the translator would be simpler.

I do understand that some projects are using the translator as SPIR-V consumer as well and this option allows to simplify their code, but it would be good if such customizations are not unique to a single consumer/backend/vendor and we don't end up with bunch of tweaks in the translator for each particular backend.

P.S. This is just my vision and it might be wrong. Probably it is not such a problem to have some consumer part customizations not in the actual SPIR-V consumers, but here, in the translator. It would be good to hear feedback from someone else

In general I don't see such customization as a big problem, we have here in the translator different flows for OpenCL 2.0 and OpenCL 1.2, right? Yet I see your point and understand it. Would also love to here other opinions.

(The story below isn't valuable for SPIR-V to LLVM IR translator project at all, but I want you to see the logic, why this option was added).
This option came from (surprise-surprise) some implementation details of SYCL compiler's driver and desire to make the feature adoptable not only for FPGA, but for other targets in JIT compilation (aka with triple spirv-unknown-unknown for which we add spirv-ext=+all option.

we have here in the translator different flows for OpenCL 2.0 and OpenCL 1.2, right?

This is for sure shared between several vendors

This option came from (surprise-surprise) some implementation details of SYCL compiler's driver and desire to make the feature adoptable not only for FPGA, but for other targets in JIT compilation (aka with triple spirv-unknown-unknown for which we add spirv-ext=+all option.

This is exactly what I'm talking about: device-/vendor-specific consumption rules are being moved into the translator. I think it is okay to outline common parts into the translator, but at the same time I wouldn't like to see bunch of vendor-specific options in this repo.

I don't have strong objections at this point: probably we can add this option as a first example and when we have the second one we can see how this can be generalized, where it goes to and how does it look like

A SPIR-V module declares capabilities, i.e. features which are used in the module. A client API environment (aka target environment) should check capabilities declared in the module against capabilities it supports. If the client API doesn't support a capability declared in the module it's "allowed to reject the module."
The EnableUsmAddrspaces option looks like we are trying adapt a SPIR-V module with USMStorageClassesINTEL capability for client API(device) which doesn't support that capability. IMO a client API should either support the capability or reject the module.
I agree with @AlexeySachkov, we should not move device specific logic into the translator.

Ok. Do you have any comments other than this one?

lib/SPIRV/SPIRVInternal.h

AlexeySachkov · 2020-06-09T18:26:01Z

@svenvh @AlexeySotkin can I ask your opinion of changing rules for already existing instructions GenericCastToPtr and PtrCastToGeneric (under the extension)

This is probably question not to the translator, but to the SPIR-V core and extension specs maintainers, i.e. what is better for extension: define new instruction or change semantic of an existing one? I guess there is no single answer to that and it should be decided case by case

and if it's possible to upstream the changes to spirv-val tool?

Again, this is probably question not for the translator, but for KhronosGroup/SPIRV-Tools. AFAIK, if you have your extension published in KhronosGroup/SPIRV-Headers there should be no problem with extending spirv-val with support for a new extension. I remember @bashbaug advised me to do so several times

MrSidims · 2020-06-18T22:12:34Z

@svenvh @AlexeySotkin can I ask your opinion of changing rules for already existing instructions GenericCastToPtr and PtrCastToGeneric (under the extension) and if it's possible to upstream the changes to spirv-val tool? If it's not OK we can just implement new instructions to reflect desired address space casts in SPIR-V.

We have changed the extension, adding two more instructions.

lib/SPIRV/SPIRVWriter.cpp

MrSidims · 2020-06-23T11:09:57Z

@AlexeySotkin @AlexeySachkov ping

AlexeySachkov

A few minor comments, LGTM overall.

I'm still not convinced that new option is actually needed, but I don't have strong objections against it either. Would be good to hear feedback from @AlexeySotkin and @svenvh

lib/SPIRV/SPIRVWriter.cpp

lib/SPIRV/libSPIRV/SPIRVInstruction.h

lib/SPIRV/libSPIRV/SPIRVModule.h

AlexeySachkov · 2020-06-24T14:28:35Z

lib/SPIRV/SPIRVReader.cpp

+        SPIRSPIRVAddrSpaceMap::rmap(T->getPointerStorageClass());
+    // If generation of SYCL USM address spaces isn't allowed in the reversed
+    // translation - transform them into global address space
+    if (!BM->isUsmAddrspacesEnabled() &&


we have here in the translator different flows for OpenCL 2.0 and OpenCL 1.2, right?

This is for sure shared between several vendors

This option came from (surprise-surprise) some implementation details of SYCL compiler's driver and desire to make the feature adoptable not only for FPGA, but for other targets in JIT compilation (aka with triple spirv-unknown-unknown for which we add spirv-ext=+all option.

This is exactly what I'm talking about: device-/vendor-specific consumption rules are being moved into the translator. I think it is okay to outline common parts into the translator, but at the same time I wouldn't like to see bunch of vendor-specific options in this repo.

I don't have strong objections at this point: probably we can add this option as a first example and when we have the second one we can see how this can be generalized, where it goes to and how does it look like

lib/SPIRV/SPIRVReader.cpp

AlexeySotkin · 2020-06-25T12:56:23Z

lib/SPIRV/SPIRVWriter.cpp

+      assert(
+          (DestAddrSpace == SPIRAS_Global || DestAddrSpace == SPIRAS_Generic) &&
+          "Casts from global_device/global_host only allowed to \
+             global/generic");
+      if (!BM->isAllowedToUseExtension(
+              ExtensionID::SPV_INTEL_usm_storage_classes)) {
+        if (DestAddrSpace == SPIRAS_Global)
+          return nullptr;
+        BOC = OpPtrCastToGeneric;
+      } else {
+        BOC = OpPtrCastToCrossWorkgroupINTEL;
+      }


Minor: this probably can (but not necessarily) be factored out to a function or lambda.

lib/SPIRV/SPIRVWriter.cpp

AlexeySotkin · 2020-06-25T12:59:45Z

The commit message should be updated. Also it would be great if you squash your commits. Thanks.

lib/SPIRV/SPIRVReader.cpp

AlexeySotkin

I'd remove the second paragraph of the commit message.
Other than that, LGTM. Thanks!

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. Co-authored-by: Viktoria Maksimova <[email protected]> Signed-off-by: Dmitry Sidorov <[email protected]> Signed-off-by: Viktoria Maksimova <[email protected]>

MrSidims · 2020-06-25T15:05:07Z

I'd remove the second paragraph of the commit message.
Other than that, LGTM. Thanks!

Okay, thank you and @AlexeySachkov for the review!

MrSidims mentioned this pull request Jun 9, 2020

[SYCL][DOC][SPIRV] Added a SPIR-V extension that introduces two new storage classes. intel/llvm#1840

Merged

AlexeySachkov reviewed Jun 9, 2020

View reviewed changes

MrSidims mentioned this pull request Jun 10, 2020

[SYCL] Add device_ptr and host_ptr intel/llvm#1864

Merged

MrSidims force-pushed the private/MrSidims/USMStorageClasses branch from 15c8347 to 1b33fbf Compare June 18, 2020 20:42

MrSidims requested a review from AlexeySachkov June 19, 2020 12:12

MrSidims force-pushed the private/MrSidims/USMStorageClasses branch from 9aaa840 to b37fac3 Compare June 22, 2020 06:54

MrSidims commented Jun 23, 2020

View reviewed changes

lib/SPIRV/SPIRVWriter.cpp Outdated Show resolved Hide resolved

AlexeySachkov reviewed Jun 24, 2020

View reviewed changes

MrSidims mentioned this pull request Jun 25, 2020

[SPIR-V] Add support for SPV_INTEL_usm_storage_classes extension intel/llvm#1985

Merged

MrSidims requested review from AlexeySotkin and AlexeySachkov June 25, 2020 12:26