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

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

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

MrSidims
Copy link
Contributor

@MrSidims MrSidims commented Jun 25, 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.

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 Author

It's a cherry-pick of KhronosGroup/SPIRV-LLVM-Translator#579

@MrSidims MrSidims force-pushed the private/MrSidims/StorageClass branch from dc20a09 to 366e59e Compare June 25, 2020 14:06
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 MrSidims force-pushed the private/MrSidims/StorageClass branch from 366e59e to 053e4d2 Compare June 25, 2020 15:12
@MrSidims
Copy link
Contributor Author

@bader can I ignore clang-format concerns or I shall apply it to spirv.hpp?

@bader
Copy link
Contributor

bader commented Jun 25, 2020

@bader can I ignore clang-format concerns or I shall apply it to spirv.hpp?

Isn't clang-formating patches required by the translator project as well?

@MrSidims
Copy link
Contributor Author

@bader can I ignore clang-format concerns or I shall apply it to spirv.hpp?

Isn't clang-formating patches required by the translator project as well?

They are required, but not firing in this case, prove: https://travis-ci.org/github/KhronosGroup/SPIRV-LLVM-Translator/jobs/701976405 . I think that is because spirv.hpp is excluded from clang-format there and it's not clang-formated in general.

@bader
Copy link
Contributor

bader commented Jun 25, 2020

@bader can I ignore clang-format concerns or I shall apply it to spirv.hpp?

Isn't clang-formating patches required by the translator project as well?

They are required, but not firing in this case, prove: https://travis-ci.org/github/KhronosGroup/SPIRV-LLVM-Translator/jobs/701976405 . I think that is because spirv.hpp is excluded from clang-format there and it's not clang-formated in general.

I see at least two solutions.

  1. We should remove copy of SPIR-V translator project from our source base and use Khronos repository version directly.
  2. Apply similar filter to our clang-format check.

@MrSidims
Copy link
Contributor Author

MrSidims commented Jun 26, 2020

I see at least two solutions.

  1. We should remove copy of SPIR-V translator project from our source base and use Khronos repository version directly.
  2. Apply similar filter to our clang-format check.

In the translator clang-format rules file is just a single line:
BasedOnStyle: LLVM
thus my theory about special rules for spirv.hpp is incorrect. But still there might be differences, with our rules, since in this repo we have wrote them manually.

@bader is it possible to merge the patch with a clang-format check failed? I see no required label.

@bader
Copy link
Contributor

bader commented Jun 26, 2020

I see at least two solutions.

  1. We should remove copy of SPIR-V translator project from our source base and use Khronos repository version directly.
  2. Apply similar filter to our clang-format check.

In the translator clang-format rules file is just a single line:
BasedOnStyle: LLVM
thus my theory about special rules for spirv.hpp is incorrect. But still there might be differences, with our rules, since in this repo we have wrote them manually.

I meant "filter" not "rule" - more specifically using this script https://github.com/intel/llvm/blob/sycl/llvm-spirv/utils/check_code_format.sh in this workflow: https://github.com/intel/llvm/blob/sycl/.github/workflows/clang-format.yml#L21.

This should fix your pre-commit checks.

@bader is it possible to merge the patch with a clang-format check failed? I see no required label.

Yes. Do you want to merge it with failing clang-format check?

@MrSidims
Copy link
Contributor Author

I see at least two solutions.

  1. We should remove copy of SPIR-V translator project from our source base and use Khronos repository version directly.
  2. Apply similar filter to our clang-format check.

In the translator clang-format rules file is just a single line:
BasedOnStyle: LLVM
thus my theory about special rules for spirv.hpp is incorrect. But still there might be differences, with our rules, since in this repo we have wrote them manually.

I meant "filter" not "rule" - more specifically using this script https://github.com/intel/llvm/blob/sycl/llvm-spirv/utils/check_code_format.sh in this workflow: https://github.com/intel/llvm/blob/sycl/.github/workflows/clang-format.yml#L21.

This should fix your pre-commit checks.

Thanks, I'll try to exclude spirv.hpp today.

@bader is it possible to merge the patch with a clang-format check failed? I see no required label.

Yes. Do you want to merge it with failing clang-format check?

Yes, since I'm not planning to apply clang-format to spirv.hpp anyway, hence there will be no changes in the code for this PR.

@bader bader merged commit a7b763b into intel:sycl Jun 26, 2020
againull pushed a commit to againull/llvm that referenced this pull request May 4, 2023
In KhronosGroup/SPIRV-LLVM-Translator@667bf13 we renamed

`-spirv-ocl-builtins-version` to `-spirv-target-env` but we missed updating a couple of spots.
Signed-off-by: Sarnie, Nick <[email protected]>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@c150b88
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.

3 participants