Remove "unreachable" instruction from LLVM IR for SYCL devices #1789

srividya-sundaram · 2020-06-01T03:02:21Z

C/C++ standard functions with __attribute__ ((__noreturn__)) in SYCL device code emit "unreachable" instruction in LLVM IR. This PR removes the "ur" instruction in LLVM IR for SYCL devices.

clang/lib/CodeGen/CGCall.cpp

erichkeane · 2020-06-02T16:50:23Z

@Savonic: Mind taking 1 more look? I'm going to approve, but I want your final thought as well.

erichkeane

Actually, I just realized there isn't a test here. Please add a CodeGen LIT test for this.

bader · 2020-06-02T17:00:23Z

C/C++ standard functions with __attribute__ ((__noreturn__)) in SYCL device code emit "unreachable" instruction in LLVM IR. This "unreachable" instruction causes Intel NEO driver and Gen "X" GPUs to hang.

Isn't this expected behavior? Maybe I don't understand the behavior of this attribute it looks like if I call "noreturn" function, it might run forever. Right?

erichkeane · 2020-06-02T17:07:02Z

C/C++ standard functions with __attribute__ ((__noreturn__)) in SYCL device code emit "unreachable" instruction in LLVM IR. This "unreachable" instruction causes Intel NEO driver and Gen "X" GPUs to hang.

Isn't this expected behavior? Maybe I don't understand the behavior of this attribute it looks like if I call "noreturn" function, it might run forever. Right?

Well, simply that the function doesn't 'return', and the optimizer is allowed to assume that it is a 'terminate' type of thing. Typically, a noreturn function must terminate the program (as never-ending loops are considered UB).

I forget the context of the bug report, but my understanding is that the driver folks didn't have a mechanism for handling this in the case of abort. Presumably, they would need to teach said drivers to abort, but I thought there wasn't a good way to do so.

Fznamznon

BTW, please add a test.

clang/lib/CodeGen/CGCall.cpp

bader · 2020-06-02T17:22:44Z

C/C++ standard functions with __attribute__ ((__noreturn__)) in SYCL device code emit "unreachable" instruction in LLVM IR. This "unreachable" instruction causes Intel NEO driver and Gen "X" GPUs to hang.

Isn't this expected behavior? Maybe I don't understand the behavior of this attribute it looks like if I call "noreturn" function, it might run forever. Right?

Well, simply that the function doesn't 'return', and the optimizer is allowed to assume that it is a 'terminate' type of thing. Typically, a noreturn function must terminate the program (as never-ending loops are considered UB).

I forget the context of the bug report, but my understanding is that the driver folks didn't have a mechanism for handling this in the case of abort. Presumably, they would need to teach said drivers to abort, but I thought there wasn't a good way to do so.

Is code comments below captures it right?

void foo() { while(1); } // UB
[[noreturn]] void foo() { while(1); } // must terminate - not UB

erichkeane · 2020-06-02T17:32:27Z

C/C++ standard functions with __attribute__ ((__noreturn__)) in SYCL device code emit "unreachable" instruction in LLVM IR. This "unreachable" instruction causes Intel NEO driver and Gen "X" GPUs to hang.

Isn't this expected behavior? Maybe I don't understand the behavior of this attribute it looks like if I call "noreturn" function, it might run forever. Right?

Well, simply that the function doesn't 'return', and the optimizer is allowed to assume that it is a 'terminate' type of thing. Typically, a noreturn function must terminate the program (as never-ending loops are considered UB).
I forget the context of the bug report, but my understanding is that the driver folks didn't have a mechanism for handling this in the case of abort. Presumably, they would need to teach said drivers to abort, but I thought there wasn't a good way to do so.

Is code comments below captures it right?
void foo() { while(1); } // UB
[[noreturn]] void foo() { while(1); } // must terminate - not UB

Infinite loops themselves are UB, see http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1528.htm

By my reading of the standard, BOTH while loops there are UB. I don't believe that the noreturn attribute changes that at all. It simply means that the compiler can optimize assuming that code after the call to a no-return function never executes.

…vices

…sycl devices

Fznamznon

I'm fine with FE changes, but I'd like @asavonic to approve first since I'm not an expert in stuff which is happening in assert test.

asavonic · 2020-06-09T12:48:03Z

Please align the PR title with the latest revision of the patch. It is no longer limited to Intel NEO driver.

srividya-sundaram · 2020-06-09T16:03:03Z

@rbegam @intel/llvm-reviewers-runtime Can you please review this PR?

sycl/test/devicelib/assert.cpp

alexbatashev

LGTM

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]>

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]>

Signed-off-by: Arvind Sudarsanam <[email protected]> Original commit: KhronosGroup/SPIRV-LLVM-Translator@a8b0c8e

srividya-sundaram added 2 commits May 31, 2020 19:45

Avoid emitting "unreachable" instruction in LLVM IR for NEO runtime

9c889ee

Fix clang format

e32592b

srividya-sundaram requested review from erichkeane and Fznamznon as code owners June 1, 2020 03:02

Fznamznon reviewed Jun 1, 2020

View reviewed changes

clang/lib/CodeGen/CGCall.cpp Outdated Show resolved Hide resolved

Fznamznon requested a review from asavonic June 1, 2020 19:07

asavonic reviewed Jun 2, 2020

View reviewed changes

clang/lib/CodeGen/CGCall.cpp Outdated Show resolved Hide resolved

clang/lib/CodeGen/CGCall.cpp Outdated Show resolved Hide resolved

Emit "unreachable" instruction for non-sycl devices only

e322c81

erichkeane reviewed Jun 2, 2020

View reviewed changes

clang-format fix

086f779

Fznamznon reviewed Jun 2, 2020

View reviewed changes

clang/lib/CodeGen/CGCall.cpp Outdated Show resolved Hide resolved

Fznamznon requested a review from asavonic June 2, 2020 17:18

Fix check for non-sycl devices

c8a778b

srividya-sundaram added 5 commits June 2, 2020 15:25

Add test to check if "unreachable" instruction is emitted for sycl de…

31ef4a2

…vices

Fix clang-format errors

b7c8345

Emit "unreachable" instruction if a function does not return for non-…

e2654d4

…sycl devices

Fix clang-format errors

e6f3d83

Fix failing test - assert.cpp

0809711

srividya-sundaram requested a review from a team as a code owner June 7, 2020 01:41

srividya-sundaram requested a review from rbegam June 7, 2020 01:41

Fznamznon reviewed Jun 8, 2020

View reviewed changes

asavonic previously approved these changes Jun 9, 2020

View reviewed changes

Fznamznon previously approved these changes Jun 9, 2020

View reviewed changes

srividya-sundaram changed the title ~~Remove "unreachable" instruction from LLVM IR for Intel NEO driver and Gen "X" GPUs~~ Remove "unreachable" instruction from LLVM IR for SYCL devices Jun 9, 2020

alexbatashev previously approved these changes Jun 9, 2020

View reviewed changes

sycl/test/devicelib/assert.cpp Outdated Show resolved Hide resolved

Remove extra space

fc6db38

srividya-sundaram dismissed stale reviews from alexbatashev, Fznamznon, and asavonic via fc6db38 June 9, 2020 16:32

srividya-sundaram requested review from elizabethandrews and premanandrao as code owners June 9, 2020 16:32

alexbatashev approved these changes Jun 9, 2020

View reviewed changes

premanandrao approved these changes Jun 9, 2020

View reviewed changes

bader merged commit 74aa9a0 into intel:sycl Jun 10, 2020

srividya-sundaram deleted the unreachable-inst-neo branch June 10, 2020 16:38

asavonic mentioned this pull request Jul 23, 2020

[SYCL] Remove noreturn function attribute #2165

Merged

bb-sycl pushed a commit that referenced this pull request Jan 18, 2023

Fix some static analyzer issues (#1789)

ed80508

Signed-off-by: Arvind Sudarsanam <[email protected]> Original commit: KhronosGroup/SPIRV-LLVM-Translator@a8b0c8e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove "unreachable" instruction from LLVM IR for SYCL devices #1789

Remove "unreachable" instruction from LLVM IR for SYCL devices #1789

Uh oh!

srividya-sundaram commented Jun 1, 2020 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erichkeane commented Jun 2, 2020

Uh oh!

erichkeane left a comment

Uh oh!

bader commented Jun 2, 2020

Uh oh!

erichkeane commented Jun 2, 2020

Uh oh!

Fznamznon left a comment

Uh oh!

Uh oh!

bader commented Jun 2, 2020

Uh oh!

erichkeane commented Jun 2, 2020

Uh oh!

Fznamznon left a comment

Uh oh!

asavonic commented Jun 9, 2020

Uh oh!

srividya-sundaram commented Jun 9, 2020

Uh oh!

Uh oh!

alexbatashev left a comment

Uh oh!

Uh oh!

Remove "unreachable" instruction from LLVM IR for SYCL devices #1789

Remove "unreachable" instruction from LLVM IR for SYCL devices #1789

Uh oh!

Conversation

srividya-sundaram commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erichkeane commented Jun 2, 2020

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented Jun 2, 2020

Uh oh!

erichkeane commented Jun 2, 2020

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bader commented Jun 2, 2020

Uh oh!

erichkeane commented Jun 2, 2020

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

asavonic commented Jun 9, 2020

Uh oh!

srividya-sundaram commented Jun 9, 2020

Uh oh!

Uh oh!

alexbatashev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

srividya-sundaram commented Jun 1, 2020 •

edited

Loading