Skip to content

[SYCL] Remove redundant build options processing #3342

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 2 commits into from
Mar 13, 2021

Conversation

vladimirlaz
Copy link
Contributor

After 86b0e8d patch extra operations with device images happen before
check if it is present in in-memory cache. For applications with small
kernels which are executed multiple times noticeable performance
degradation is seen for host code. That was done to get build options
stored in the kernel image and use them as in-memory cache key. At the
same time kernel image (where these options are taken from) is used in
cache key so it is reasonable to use only build options which are
specified in SYCL API and/or environment variables as separate cache
key.

Getting build options from kernel image is moved back to build
operation which happens only if built program is missed in in-memory
cache.

After 86b0e8d patch extra operations with device images happen before
check if it is present in in-memory cache. For applications with small
kernels which are executed multiple times noticeable performance
degradation is seen for host code. That was done to get build options
stored in the kernel image and use them as in-memory cache key. At the
same time kernel image (where these options are taken from) is used in
cache key so it is reasonable to use only build options which are
specified in SYCL API and/or environment variables as separate cache
key.

Getting build options from kernel image is moved back to build
operation which happens only if built program is missed in in-memory
cache.
@vladimirlaz vladimirlaz requested a review from kbobrovs as a code owner March 11, 2021 13:04
@vladimirlaz
Copy link
Contributor Author

/summary:run

@kbobrovs
Copy link
Contributor

  • I believe there can be a situation when the same binary image (.spv) participates twice in the fat binary, but with different compile options (e.g. debug vc no debug). If I understand correctly, the cache key will be the same for both, so RT will compile and cache one variant (debug or no debug), and subsequently use cached native binary for both debug and non-debug "source" .spv. This does not seem right.
  • I think a test should be added which catches the reported regression.

@vladimirlaz
Copy link
Contributor Author

As discussed offline TODO: mark was added to ensure that the mechanism will work once added posibillity to access several versions of the same kernel built with different options.

@kbobrovs
Copy link
Contributor

@vladimirlaz, please address this one too:

I think a test should be added which catches the reported regression.

@vladimirlaz
Copy link
Contributor Author

hould be added which catches the reported

The problem was caught by benchmarks and they can be used as regression test. There is no benchmark-style test in LIT infra.

@kbobrovs
Copy link
Contributor

@vladimirlaz, can a non-benchmark test be created, which enables PI tracing and checks that recompilation and/or reusing from cache happens expected number of times? This should belong to llvm-test-suite though, as it is end-to-end. So not a blocker for this PR.

@bader bader merged commit 8f65cd4 into intel:sycl Mar 13, 2021
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Mar 15, 2021
* upstream/sycl:
  Add ITT stubs and wrappers for SPIR-V devices (intel#3279)
  [SYCL] Add zero argument version of buffer::reinterpret() for SYCL 2020 (intel#3333)
  [SYCL] Restore old behavior of get() method (intel#3356)
  [Driver][SYCL][FPGA] Improve FPGA AOT when using Triple (intel#3330)
  [SYCL] Revert support for pinned_host_memory extension in Level-Zero backend. Make it a NOP (intel#3349)
  [SYCL] Remove redundant build options processing (intel#3342)
  [SYCL][NFC] Factor out GenXIntrinsics git repo tag. (intel#3351)
  [SYCL] Add coarse-grained debug aid for finding imbalance in Level-Zero alloc/free (intel#3334)
  [SYCL] Sub-group load/store for raw pointers (intel#3255)
  [SYCL] Fix more unused variables warnings (intel#3352)
  [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process  (intel#3338)
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Mar 19, 2021
* upstream/sycl: (1804 commits)
  [SYCL] SYCL 2020 backend interoperability part 1 (intel#3354)
  [Driver][SYCL] Address issue when unbundling for non-FPGA archive (intel#3366)
  [SYCL][Doc] Update compiler options descriptions (intel#3340)
  [SYCL] Update ABI dump tool to disable checks with libcxx by default (intel#3370)
  [SYCL] Update the way we handle -sycl-std= based on community review feedback (intel#3371)
  [SYCL] Move tests to llvm-test-suite (intel#3359)
  [SYCL][PI][L0] Revert copy batching from intel#3232 (intel#3363)
  [SYCL] Remove unsupported option.
  [SYCL] Fix pragma setting in sycl-post-link (intel#3358)
  [SYCL] Add ITT annotation instructions (intel#3299)
  [SYCL] Propagate attributes of original kernel to wrapper kernel generated for range-rounding (intel#3306)
  [BuildBot] Uplift GPU RT version for Linux to 21.09.19150 (intel#3316)
  [SYCL] Retain PI events until they have signaled (intel#3350)
  [SYCL] Add caching when using interop constructor (intel#3327)
  Add ITT stubs and wrappers for SPIR-V devices (intel#3279)
  [SYCL] Add zero argument version of buffer::reinterpret() for SYCL 2020 (intel#3333)
  [SYCL] Restore old behavior of get() method (intel#3356)
  [Driver][SYCL][FPGA] Improve FPGA AOT when using Triple (intel#3330)
  [SYCL] Revert support for pinned_host_memory extension in Level-Zero backend. Make it a NOP (intel#3349)
  [SYCL] Remove redundant build options processing (intel#3342)
  ...
@vladimirlaz vladimirlaz deleted the jit_cache_bug_1 branch April 23, 2021 06:27
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