Skip to content

[SYCL] Deny support of SPV_INTEL_usm_storage_classes for all targets … #1986

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 5 commits into from
Jun 29, 2020

Conversation

MrSidims
Copy link
Contributor

…but FPGA

Disable SPV_INTEL_usm_storage_classes by default since it adds new
storage classes that represent global_device and global_host address
spaces, which are not supported for all targets. With the extension
disabled the storage classes will be lowered to CrossWorkgroup storage
class that is mapped to just global address space.

The extension is enabled only for FPGA with a flag 'enable-usm-address-spaces'
passed to the driver.

Signed-off-by: Dmitry Sidorov [email protected]

@MrSidims
Copy link
Contributor Author

NOTE: #1985 - need to be merged first

MrSidims added 2 commits June 26, 2020 17:37
…but FPGA

Disable SPV_INTEL_usm_storage_classes by default since it adds new
storage classes that represent global_device and global_host address
spaces, which are not supported for all targets. With the extension
disabled the storage classes will be lowered to CrossWorkgroup storage
class that is mapped to just global address space.

The extension is enabled only for FPGA with a flag 'enable-usm-address-spaces'
passed to the driver.

Signed-off-by: Dmitry Sidorov <[email protected]>
Signed-off-by: Dmitry Sidorov <[email protected]>
@MrSidims MrSidims force-pushed the private/MrSidims/DriverExt branch from ba2ef69 to dc84a4c Compare June 26, 2020 14:38
Copy link
Contributor

@AGindinson AGindinson left a comment

Choose a reason for hiding this comment

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

The patch mostly LGTM, what's probably lacking is a positive test of -fenable-usm-address-spaces.

@MrSidims
Copy link
Contributor Author

The patch mostly LGTM, what's probably lacking is a positive test of -fenable-usm-address-spaces.

I was thinking I had it, thanks for pointing out!

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

@AGindinson I have added a positive test case in sycl-offload.c. Do you think it's necessary to add it in sycl-offload-intelfpga.cpp as well? I think the last test is more about aocx generation than about options.

@AGindinson
Copy link
Contributor

I have added a positive test case in sycl-offload.c. Do you think it's necessary to add it in sycl-offload-intelfpga.cpp as well? I think the last test is more about aocx generation than about options.

I agree, sycl-offload.c should be sufficient

Signed-off-by: Dmitry Sidorov <[email protected]>
Signed-off-by: Dmitry Sidorov <[email protected]>
@MrSidims MrSidims requested review from AGindinson and mdtoguchi June 28, 2020 19:49
Copy link
Contributor

@mdtoguchi mdtoguchi left a comment

Choose a reason for hiding this comment

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

LGTM

@bader bader merged commit 15e37a8 into intel:sycl Jun 29, 2020
jsji pushed a commit that referenced this pull request May 15, 2023
Currently, we always convert SPIR-V bultins to globals for forward translation and to functions for reverse translation.

I have a use case where I want to keep them as globals for reverse translation, so I added this mode.

Implementations for both cases already existed, I just consolidated them and added the option.

Signed-off-by: Sarnie, Nick <[email protected]>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@730eaf0
jsji pushed a commit that referenced this pull request May 15, 2023
Currently, we always convert SPIR-V bultins to globals for forward translation and to functions for reverse translation.

I have a use case where I want to keep them as globals for reverse translation, so I added this mode.

Implementations for both cases already existed, I just consolidated them and added the option.

Signed-off-by: Sarnie, Nick <[email protected]>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@730eaf0
jsji pushed a commit that referenced this pull request May 16, 2023
Currently, we always convert SPIR-V bultins to globals for forward translation and to functions for reverse translation.

I have a use case where I want to keep them as globals for reverse translation, so I added this mode.

Implementations for both cases already existed, I just consolidated them and added the option.

Signed-off-by: Sarnie, Nick <[email protected]>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@730eaf0
jsji pushed a commit that referenced this pull request May 16, 2023
Currently, we always convert SPIR-V bultins to globals for forward translation and to functions for reverse translation.

I have a use case where I want to keep them as globals for reverse translation, so I added this mode.

Implementations for both cases already existed, I just consolidated them and added the option.

Signed-off-by: Sarnie, Nick <[email protected]>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@730eaf0
jsji pushed a commit to sys-ce-bb/llvm that referenced this pull request May 16, 2023
)

Currently, we always convert SPIR-V bultins to globals for forward translation and to functions for reverse translation.

I have a use case where I want to keep them as globals for reverse translation, so I added this mode.

Implementations for both cases already existed, I just consolidated them and added the option.

Signed-off-by: Sarnie, Nick <[email protected]>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@730eaf0
@jsji jsji mentioned this pull request May 16, 2023
jsji pushed a commit that referenced this pull request May 16, 2023
Currently, we always convert SPIR-V bultins to globals for forward translation and to functions for reverse translation.

I have a use case where I want to keep them as globals for reverse translation, so I added this mode.

Implementations for both cases already existed, I just consolidated them and added the option.

Signed-off-by: Sarnie, Nick <[email protected]>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@730eaf0
This was referenced May 16, 2023
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.

4 participants