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] Materialize shadow local variables for byval arguments before use #2200

Merged
merged 1 commit into from
Jul 29, 2020

Conversation

againull
Copy link
Contributor

Currently shadow local variables for byval arguments are used directly.
In some cases this leads to invalid casts between address spaces.
Materialize these local variables before using to fix this problem.

Signed-off-by: Artur Gainullin [email protected]

@againull againull requested a review from bader as a code owner July 29, 2020 05:58
Currently shadow local variables for byval arguments are used directly.
In some cases this leads to invalid casts between address spaces.
Materialize these local variables before using to fix this problem.

Signed-off-by: Artur Gainullin <[email protected]>
@againull againull force-pushed the addrspace_cast_fix branch from 01cc03f to 01dbe0a Compare July 29, 2020 06:03
@againull againull requested a review from kbobrovs July 29, 2020 06:30
@kbobrovs
Copy link
Contributor

This adds more overhead.
I wonder if the shadow->private copy can be added only where the invalid address space casts can't be fixed otherwise w/o copying.

In some cases this leads to invalid casts between address spaces.

Do you have an example of such casts?

@againull
Copy link
Contributor Author

This adds more overhead.
I wonder if the shadow->private copy can be added only where the invalid address space casts can't be fixed otherwise w/o copying.

In some cases this leads to invalid casts between address spaces.

Do you have an example of such casts?

I have added new lit test which demonstrates such case -> illegal cast from local address space to private address space is generated without the fix.
Yes, this adds an overhead, but in all other cases (allocas, results of instructions with side effects and so on) we do materialization, I think number of byval arguments is much less than other cases (which need to be shadowed in local memory) and overhead is negligible.

@bader bader merged commit c6c2ecd into intel:sycl Jul 29, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Jul 30, 2020
…rogram

* upstream/sycl: (609 commits)
  [SYCL] Fix fail in the post commit testing (intel#2210)
  [SYCL] Materialize shadow local variables for byval arguments before use (intel#2200)
  [SYCL] Support lambda functions passed to reduction (intel#2190)
  [SYCL][USM] Improve USM Allocator. (intel#2026)
  [SYCL] Disallow mutable lambdas (intel#1785)
  [SYCL][ESIMD] Setup compilation pipeline for ESIMD (intel#2134)
  [SYCL] Fix not found kernel due to empty kernel name when using set_arg(s) (intel#2181)
  [SYCL] Fixed check for set_arg (intel#2203)
  Refactor indirect access calls to minimize invocations. (intel#2185)
  [SYCL][NFC] Fix potential null-pointer access (intel#2197)
  [SYCL] Propagate attributes from transitive calls to kernel (intel#1878)
  [SYCL] Fix warnings from static analysis tool (intel#2193)
  [SYCL][NFC] Fix ac_float test for compilation with FE optimizations (intel#2184)
  [GitHub Actions] Uplift clang-format version to 10 (intel#2194)
  [SYCL][ESIMD] Pass to replace simd* parameters with native llvm vectors. (intel#2097)
  [SYCL][NFC] Fixed SYCL_PI_TRACE output while selecting a device. (intel#2192)
  [SYCL][FPGA] New spec for controlling load-store units in FPGAs (intel#2158)
  [SYCL][Doc] Clarify reqd_sub_group_size (intel#2103)
  [SYCL] Remove noreturn function attribute (intel#2165)
  [SYCL] Aligned set_arg behaviour with SYCL specification (intel#2159)
  ...
@againull againull deleted the addrspace_cast_fix branch December 3, 2022 00:03
jsji pushed a commit that referenced this pull request Nov 9, 2023
It's still possible to rewrite it, but it (as well as the problem it
addresses) will become obsolete when
https://discourse.llvm.org/t/rfc-remove-most-constant-expressions/63179
is fully implemented.

Signed-off-by: Sidorov, Dmitry <[email protected]>

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