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] Improve SYCL_DEVICE_ALLOWLIST #3826

Merged

Conversation

dm-vodopyanov
Copy link
Contributor

@dm-vodopyanov dm-vodopyanov commented May 26, 2021

This patch adds more stability to SYCL_DEVICE_ALLOWLIST:

  1. Introduce 3 new keys BackendName, DeviceType and
    DeviceVendorId which should be used instead of DeviceName and
    PlatformName. These 3 new keys are more stable.

  2. Refactor the implementation of SYCL_DEVICE_ALLOWLIST to

    • make it more stable and clean
    • fix std::bad_alloc crash
    • make the code testable
  3. Add unit tests for parsing SYCL_DEVICE_ALLOWLIST value and for
    functionality which allows device to be accepted or to be rejected.

This patch adds more stability to SYCL_DEVICE_ALLOWLIST:

1. Introduce 3 new keys `BackendName`, `DeviceType` and
`DeviceVendorId` which should be used instead of `DeviceName` and
`PlatformName`. These 3 new keys are more stable.
2. Refactor the implementation of SYCL_DEVICE_ALLOWLIST to make it more
stable, to fix std::bad_alloc crash, and to make the code testable
3. Add unit tests for parsing SYCL_DEVICE_ALLOWLIST value and for
functionality which allows device to use or reject it.
@dm-vodopyanov dm-vodopyanov requested review from bader, pvchupin and a team as code owners May 26, 2021 18:28
@dm-vodopyanov dm-vodopyanov requested a review from againull May 26, 2021 18:28
@dm-vodopyanov
Copy link
Contributor Author

Working on fixing Jenkins/Precommit.

sycl/doc/EnvironmentVariables.md Outdated Show resolved Hide resolved
sycl/source/detail/platform_impl.cpp Outdated Show resolved Hide resolved
sycl/source/detail/platform_impl.hpp Outdated Show resolved Hide resolved
"BackendName:level_zero,SomeUnsupportedKey:gpu");
} catch (sycl::runtime_error const &e) {
EXPECT_EQ(e.what(), std::string("Unrecognized key in SYCL_DEVICE_ALLOWLIST "
"-30 (CL_INVALID_VALUE)"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why "-30 (CL_INVALID_VALUE)"?
Looks like some error code is mapped to OpenCL specific enumeration. The reason is unclear.

Copy link
Contributor Author

@dm-vodopyanov dm-vodopyanov Jun 1, 2021

Choose a reason for hiding this comment

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

Yes, AFAIS, this is a common situation in RT right now: there is no RT-only error codes, so everywhere in the code, even for non-backend related things, PI_ error codes are used, which are casted to CL_ error codes.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is very annoying bug, which we should fix sooner rather than later.
Please, make sure it's addressed separately.

Copy link
Contributor Author

@dm-vodopyanov dm-vodopyanov Jun 1, 2021

Choose a reason for hiding this comment

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

Will create a tracker. @vladimirlaz, as you work closely to SYCL2020 exceptions, do you see this issue be reproducible? (showing PI_*** or CL_*** after throwing any exception)

Copy link
Contributor

Choose a reason for hiding this comment

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

@dm-vodopyanov CL_*** format is using now

@dm-vodopyanov
Copy link
Contributor Author

Failures in Jenkins/Precommit should gone after intel/llvm-test-suite#304

@dm-vodopyanov
Copy link
Contributor Author

@pvchupin, @againull, please review.

@dm-vodopyanov dm-vodopyanov requested a review from bader June 1, 2021 11:24
bader
bader previously approved these changes Jun 1, 2021
sycl/source/detail/allowlist.cpp Outdated Show resolved Hide resolved
sycl/source/detail/allowlist.cpp Outdated Show resolved Hide resolved
@againull againull self-requested a review June 2, 2021 08:47
@againull
Copy link
Contributor

againull commented Jun 2, 2021

Overall patch looks good to me, just several minor comments

@dm-vodopyanov dm-vodopyanov requested review from againull and bader June 2, 2021 20:11
againull
againull previously approved these changes Jun 2, 2021
@againull againull self-requested a review June 3, 2021 16:04
@againull againull merged commit 9216b49 into intel:sycl Jun 3, 2021
@bader
Copy link
Contributor

bader commented Jun 4, 2021

@dm-vodopyanov, please, fix post-commit fails in "no-assertions" configuration.
https://github.com/intel/llvm/runs/2740297660?check_suite_focus=true

alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Jun 4, 2021
* sycl: (320 commits)
  [SYCL] Silence a "local variable is initialized but not referenced" warning; NFC (intel#3870)
  [SYCL] Improve SYCL_DEVICE_ALLOWLIST (intel#3826)
  [SPIR-V] Change return value of mapType function (intel#3871)
  [SYCL] Fix post-commit failure in handler.hpp from unused-parameters. (intel#3874)
  [Driver][SYCL] Do not imply defaultlib msvcrt for Linux based driver on Windows (intel#3827)
  [SYCL] Unique stable name rebase (intel#3835)
  [SYCL] Align behavior of empty command groups with SYCL2020 (intel#3822)
  [SYCL][ESIMD] Make typenames and constants consistent with SYCL API style. (intel#3850)
  [SYCL] Allow __failed_assertion to support libstdc++-11 (intel#3774)
  [SYCL] Refactor stream class handing implementation (intel#3646)
  [SYCL] Fix syntax error introduced in intel#3401 (intel#3861)
  [SYCL] SYCL 2020 sub_group algorithms (intel#3786)
  [Buildbot][NFC] Add option to use LLD as linker (intel#3866)
  Revert "Emit correct location lists with basic block sections."
  [SPIRITTAnnotations] Fix debug info for ITT calls. (intel#3829)
  [SYCL][Doc] Fix build of Sphinx docs (intel#3863)
  [SYCL][FPGA][NFC] Tidy up intel_fpga_reg codegen test (intel#3810)
  [CODEOWNERS] Fix SPIRITTAnnnotations tests ownership (intel#3859)
  [SYCL][ESIMD] Host-compile simd.cpp test, fix errors & warnings. (intel#3846)
  [SYCL] Store pointers to memory allocations instead of iterators (intel#3860)
  ...
dm-vodopyanov added a commit to dm-vodopyanov/llvm that referenced this pull request Jun 4, 2021
bader pushed a commit that referenced this pull request Jun 4, 2021
@dm-vodopyanov dm-vodopyanov deleted the private/dvodopya/improve-allowlist branch February 10, 2022 14:06
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