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] Remove noreturn function attribute #2165

Merged
merged 1 commit into from
Jul 28, 2020

Conversation

asavonic
Copy link
Contributor

@asavonic asavonic commented Jul 23, 2020

This is a follow-up to:
74aa9a0 [SYCL] Remove "unreachable" instruction from LLVM IR for SYCL
devices (#1789)

Noreturn function attribute gives LLVM optimization passes an
opportunity to re-structure code and insert an unreachable
instruction.

This happens when optimizations are turned on by
-fsycl-enable-optimizations, so noreturn function attribute must be
removed.

Signed-off-by: Andrew Savonichev [email protected]

Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

@asavonic , can you please add a test where this change shows making a difference?

@srividya-sundaram
Copy link
Contributor

Just summarizing my understanding to make sure I got it right:
In the initial PR, we prevented emitting an "unreachable" instruction in LLVM-IR, if device code (containing a no-return function) was being compiled for SYCL devices.

If a function is marked as noreturn, it has to terminate. And since SYCL does not support termination in device code, we detect no-return functions in device code and remove the no-return attributes from them?

Also, from clang -cc1 help:
-fsycl-enable-optimizations : Experimental flag enabling standard optimization in the front-end.
Can you share how this new change is better than the previous one?
The flag's description is not very helpful.

@elizabethandrews
Copy link
Contributor

It looks like the only change is the removal of the attributes as well?

@elizabethandrews
Copy link
Contributor

Oh I see. Just preventing the call to EmitUnreachable( ) wasn't enough. You have to remove the attributes as well to prevent optimizations passes from emitting this. That's why the second RUN line in test.

Can you update your PR description. The current one is confusing.

@elizabethandrews
Copy link
Contributor

elizabethandrews commented Jul 24, 2020

@asavonic , can you please add a test where this change shows making a difference?

I think the second run line with optimizations enabled is the test for this change. From what I understand, this change is required to ensure unreachable instruction is not emitted even after optimization @asavonic please confirm

@premanandrao
Copy link
Contributor

@asavonic , can you please add a test where this change shows making a difference?

I think the second run line with optimizations enabled is the test for this change. From what I understand, this change is required to ensure unreachable instruction is not emitted even after optimization @asavonic please confirm

This makes sense, but it wasn't clear. In this case, can @asavonic add a comment line to the test to specify what it tests?

Fznamznon
Fznamznon previously approved these changes Jul 27, 2020
This is a follow-up to:
74aa9a0 [SYCL] Remove "unreachable" instruction from LLVM IR for SYCL
devices (intel#1789)

Noreturn function attribute gives LLVM optimization passes an
opportunity to re-structure code and insert an unreachable
instruction.

This happens when optimizations are turned on by
-fsycl-enable-optimizations, so noreturn function attribute must be
removed.

Signed-off-by: Andrew Savonichev <[email protected]>
@asavonic asavonic force-pushed the private/asavonic/remove-unreachable branch from 2e8d795 to aeec9da Compare July 27, 2020 17:29
@asavonic
Copy link
Contributor Author

@asavonic , can you please add a test where this change shows making a difference?

Added a check for a noreturn attribute, and another RUN line with optimizations turned on.

@asavonic
Copy link
Contributor Author

@asavonic , can you please add a test where this change shows making a difference?

I think the second run line with optimizations enabled is the test for this change. From what I understand, this change is required to ensure unreachable instruction is not emitted even after optimization @asavonic please confirm

Right. I clarified this in the commit message.

@bader bader requested review from Fznamznon and premanandrao July 27, 2020 17:38
@bader bader merged commit c93fa93 into intel:sycl Jul 28, 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)
  ...
jsji pushed a commit that referenced this pull request Oct 5, 2023
… matrix (#2165)

Implement translation via SPIR-V friendly calls, as:

the LLVM instructions are not capable to accept target extension types;
cooperative matrix is an opaque object and accessing elements is implementation defined, hence we can't use GEP to which AccessChain naturally maps, since GEP has a different meaning.
As for now some BE would need to recognize and define what to do with a call to __spirv_AccessChain(matrix, index). Better option is to map such SPIR-V to an intrinsic or define an appropriate type in LLVM (hence defining rules for GEP and other instructions) , but it's off the table now.

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

6 participants