Skip to content

[SYCL] Do not emit "llvm.used" to output module(s) in the post link tool #2995

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.

Sign up for GitHub

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
Jan 6, 2021
Merged

[SYCL] Do not emit "llvm.used" to output module(s) in the post link tool #2995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d399540
[SYCL] Do not emit "llvm.used" to output module(s) in the post link tool
sndmitriev Jan 6, 2021
3f55873
Corrected assertion message
sndmitriev Jan 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions llvm/test/tools/sycl-post-link/erase_used.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
; This test checks that the post-link tool does not add "llvm.used" global to
; the output modules when splitting kernels.
;
; RUN: sycl-post-link -split=kernel -S %s -o %T/files.table
; RUN: FileCheck %s -input-file=%T/files_0.ll
; RUN: FileCheck %s -input-file=%T/files_1.ll

target triple = "spir64-unknown-unknown-sycldevice"

; CHECK-NOT: llvm.used
@llvm.used = appending global [2 x i8*] [i8* bitcast (void ()* @foo to i8*), i8* bitcast (void ()* @bar to i8*)], section "llvm.metadata"

define weak_odr spir_kernel void @foo() {
ret void
}

define weak_odr spir_kernel void @bar() {
ret void
}
9 changes: 9 additions & 0 deletions llvm/tools/sycl-post-link/sycl-post-link.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,15 @@ int main(int argc, char **argv) {
Err.print(argv[0], errs());
return 1;
}

// Special "llvm.used" variable which holds references to global values in the
// module is known to cause problems for tools which run later in pipeline, so
Copy link
Contributor

Choose a reason for hiding this comment

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

@sndmitriev, could you clarify what kind of problems might be caused, please?

Copy link
Contributor Author

@sndmitriev sndmitriev Jan 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

After linking device bitcode llvm.used holds references to the kernels that are defined in the device image. But after splitting device image into separate kernels (with -fsycl-device-code-split=per_kernel option) we may end up with having references to kernel declaration originating from “llvm.used” in the IR that is passed to llvm-spirv tool, and these declarations cause an assertion in llvm-spirv

$ cat test.ll
target triple = "spir64-unknown-unknown-sycldevice"

@llvm.used = appending global [2 x i8*] [i8* bitcast (void ()* @foo to i8*), i8* bitcast (void ()* @bar to i8*)], section "llvm.metadata"

declare spir_kernel void @foo()

define spir_kernel void @bar() {
  ret void
}
$ llvm-as -o test.bc test.ll
$ llvm-spirv -o test.spv -spirv-max-version=1.1 -spirv-debug-info-version=legacy -spirv-allow-extra-diexpressions -spirv-ext=+all,-SPV_INTEL_usm_storage_classes test.bc
llvm-spirv: …/llvm/llvm-spirv/lib/SPIRV/SPIRVWriter.cpp:3438: void SPIRV::LLVMToSPIRV::transFPContract(): Assertion `FPC != FPContract::UNDEF' failed.
…

Copy link
Contributor

@AlexeySachkov AlexeySachkov Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm a bit confused with the reasoning.

That FP-contract related assertion seems like an unhandled situation in the translator which should be fixed there.
However, I do agree that @llvm.used might cause problems with the translation, because it contains function pointers and can't be translated into SPIR-V without corresponding extension (SPV_INTEL_function_pointers) enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not argue with that, the problem needs to be fixed in the translator as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

How do we get this metadata in device code? It's not emitted in SYCL mode: aa35161. Is it emitted in other modes?
If so, shouldn't we drop @llvm.used during FE IR generation?

Anyway, it would be nice if we add clarification with reasoning to drop @llvm.used in sycl-post-link tool from this comment: #2995 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do we get this metadata in device code?

"llvm.used" variable may appear in the IR after linking with "deps" bitcode file (please see #2872 for more details).

Anyway, it would be nice if we add clarification with reasoning to drop @llvm.used in sycl-post-link tool from this comment: #2995 (comment).

I have created #3012 to address your comment. Please review it.

// remove it from the module before perfroming any other actions.
if (GlobalVariable *GV = MPtr->getGlobalVariable("llvm.used")) {
assert(GV->user_empty() && "unexpected llvm.used users");
GV->eraseFromParent();
}

if (OutputFilename.getNumOccurrences() == 0)
OutputFilename = (Twine(sys::path::stem(InputFilename)) + ".files").str();

Expand Down