-
Notifications
You must be signed in to change notification settings - Fork 271
Implement SPV_KHR_abort extension #3691
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
Changes from 4 commits
d209681
9493cdf
0b02b53
f541009
c5de6a4
b80d64e
0360938
e15f699
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2333,10 +2333,27 @@ LLVMToSPIRVBase::transValueWithoutDecoration(Value *V, SPIRVBasicBlock *BB, | |
| return mapValue(V, BI); | ||
| } | ||
|
|
||
| if (dyn_cast<UnreachableInst>(V)) | ||
| if (isa<UnreachableInst>(V)) { | ||
| // SPV_KHR_abort: OpAbortKHR is itself a block terminator. If the LLVM IR | ||
| // appends a trailing 'unreachable' after the abort call (the canonical | ||
| // pattern for noreturn calls), do not emit a second SPIR-V terminator. | ||
| if (auto *Last = BB->getTerminateInstr()) | ||
| if (Last->getOpCode() == OpAbortKHR) | ||
| return mapValue(V, const_cast<SPIRVInstruction *>(Last)); | ||
| return mapValue(V, BM->addUnreachableInst(BB)); | ||
| } | ||
|
|
||
| if (auto *RI = dyn_cast<ReturnInst>(V)) { | ||
| // SPV_KHR_abort: OpAbortKHR is itself a SPIR-V block terminator. If the | ||
| // LLVM IR appends a trailing `ret void` after the abort call, do not emit | ||
| // a second SPIR-V terminator. We deliberately limit this suppression to | ||
| // `ret void`: if a non-void function ends with `ret <value>` after | ||
| // OpAbortKHR, fall through so that the value is still translated and the | ||
| // user sees a normal SPIR-V validation error rather than silently | ||
| // dropping the return value. | ||
| if (auto *Last = BB->getTerminateInstr(); | ||
| !RI->getReturnValue() && Last && Last->getOpCode() == OpAbortKHR) | ||
| return mapValue(V, const_cast<SPIRVInstruction *>(Last)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure this is enough to handle all the cases. For example: This would be invalid, because we would have an instruction after a terminator. As far as I can tell, nothing would prevent translating the call to @llvm.lifetime.end() after the OpAbortKHR, which is a terminator. Am I right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maarquitos14, I've moved the tests you've shared from the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM! Thanks :) |
||
| if (auto *RV = RI->getReturnValue()) { | ||
| if (auto *II = dyn_cast<IntrinsicInst>(RV)) { | ||
| if (II->getIntrinsicID() == Intrinsic::frexp) { | ||
|
|
@@ -5131,9 +5148,10 @@ SPIRVValue *LLVMToSPIRVBase::transIntrinsicInst(IntrinsicInst *II, | |
| case Intrinsic::invariant_start: | ||
| case Intrinsic::invariant_end: | ||
| case Intrinsic::dbg_label: | ||
| // llvm.trap and llvm.debugtrap intrinsics are not implemented. But for now | ||
| // don't crash. This change is pending the trap/abort intrinsic | ||
| // implementation. | ||
| // TODO: lower llvm.trap / llvm.ubsantrap / llvm.debugtrap to OpAbortKHR | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we do this in this patch too? Seems very related.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maarquitos14 Maybe not to overload this PR, we can follow-up in a separate one (together with Reader part update for default/non-friendly IR path). What do you think?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I won't block this PR for this. If you want to follow-up in a separate PR, go for it. |
||
| // (with a null/undefined Message) when the SPV_KHR_abort extension is | ||
| // enabled. For now we silently drop these intrinsics so that the translator | ||
| // doesn't crash on input that contains them. | ||
| case Intrinsic::trap: | ||
| case Intrinsic::ubsantrap: | ||
| case Intrinsic::debugtrap: | ||
|
vmaksimo marked this conversation as resolved.
Outdated
|
||
|
|
@@ -6902,6 +6920,16 @@ LLVMToSPIRVBase::transBuiltinToInstWithoutDecoration(Op OC, CallInst *CI, | |
| auto BArgs = transValue(getArguments(CI), BB); | ||
| return BM->addControlBarrierInst(BArgs[0], BArgs[1], BArgs[2], BB); | ||
| } break; | ||
| case OpAbortKHR: { | ||
| BM->getErrorLog().checkError( | ||
| BM->isAllowedToUseExtension(ExtensionID::SPV_KHR_abort), | ||
| SPIRVEC_RequiresExtension, "SPV_KHR_abort\n"); | ||
| BM->getErrorLog().checkError( | ||
| CI->arg_size() == 1, SPIRVEC_InvalidInstruction, | ||
| "__spirv_AbortKHR must be called with exactly one Message argument\n"); | ||
| auto *Msg = transValue(CI->getArgOperand(0), BB); | ||
|
vmaksimo marked this conversation as resolved.
|
||
| return BM->addAbortKHRInst(Msg, BB); | ||
| } break; | ||
| case OpGroupAsyncCopy: { | ||
| auto BArgs = transValue(getArguments(CI), BB); | ||
| return BM->addAsyncGroupCopy(BArgs[0], BArgs[1], BArgs[2], BArgs[3], | ||
|
|
@@ -7592,7 +7620,8 @@ bool runSpirvBackend(Module *M, std::string &Result, std::string &ErrMsg, | |
| SPIRV::ExtensionID::SPV_INTEL_function_pointers, | ||
| SPIRV::ExtensionID::SPV_KHR_shader_clock, | ||
| SPIRV::ExtensionID::SPV_KHR_cooperative_matrix, | ||
| SPIRV::ExtensionID::SPV_KHR_non_semantic_info}; | ||
| SPIRV::ExtensionID::SPV_KHR_non_semantic_info, | ||
| SPIRV::ExtensionID::SPV_KHR_abort}; | ||
| // The fallback for the Triple value. | ||
| static const std::string DefaultTriple = "spirv64-unknown-unknown"; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 9268f3057354a2cb65991ba5f38b16d81e803692 | ||
| ad9184e76a66b1001c29db9b0a3e87f646c64de0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| ; Negative tests for the SPIR-V translator's handling of `__spirv_AbortKHR`. | ||
| ; OpAbortKHR takes exactly one Message operand; calls with any other arity must | ||
| ; be rejected by the translator with a clear diagnostic rather than producing | ||
| ; ill-formed SPIR-V. | ||
|
|
||
| ; RUN: split-file %s %t | ||
|
|
||
| ; RUN: llvm-as %t/no-args.ll -o %t/no-args.bc | ||
| ; RUN: not llvm-spirv %t/no-args.bc --spirv-ext=+SPV_KHR_abort -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-ARG-COUNT | ||
|
|
||
| ; RUN: llvm-as %t/two-args.ll -o %t/two-args.bc | ||
| ; RUN: not llvm-spirv %t/two-args.bc --spirv-ext=+SPV_KHR_abort -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-ARG-COUNT | ||
|
|
||
| ; CHECK-ARG-COUNT: InvalidInstruction | ||
| ; CHECK-ARG-COUNT: __spirv_AbortKHR must be called with exactly one Message argument | ||
|
|
||
| ;--- no-args.ll | ||
| target triple = "spir64-unknown-unknown" | ||
|
|
||
| define spir_func void @abort_no_args() { | ||
| entry: | ||
| call spir_func void @_Z16__spirv_AbortKHRv() | ||
| ret void | ||
| } | ||
|
|
||
| declare spir_func void @_Z16__spirv_AbortKHRv() | ||
|
|
||
| ;--- two-args.ll | ||
| target triple = "spir64-unknown-unknown" | ||
|
|
||
| define spir_func void @abort_two_args(i32 %a, i32 %b) { | ||
| entry: | ||
| call spir_func void @_Z16__spirv_AbortKHRii(i32 %a, i32 %b) | ||
| ret void | ||
| } | ||
|
|
||
| declare spir_func void @_Z16__spirv_AbortKHRii(i32, i32) |
|
vmustya marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| ; RUN: llvm-as %s -o %t.bc | ||
| ; RUN: llvm-spirv %t.bc -o %t.spv --spirv-ext=+SPV_KHR_abort | ||
| ; RUN: llvm-spirv %t.spv -o %t.spt --to-text | ||
| ; RUN: FileCheck < %t.spt %s --check-prefix=CHECK-SPIRV | ||
| ; RUN: llvm-spirv %t.spv -o %t.rev.bc -r --spirv-target-env=SPV-IR | ||
| ; RUN: llvm-dis %t.rev.bc -o %t.rev.ll | ||
| ; RUN: FileCheck < %t.rev.ll %s --check-prefix=CHECK-LLVM | ||
| ; RUN: spirv-val %t.spv | ||
|
|
||
| ; RUN: not llvm-spirv %t.bc 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR | ||
| ; CHECK-ERROR: RequiresExtension: Feature requires the following SPIR-V extension: | ||
| ; CHECK-ERROR-NEXT: SPV_KHR_abort | ||
|
|
||
| target datalayout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-n8:16:32:64" | ||
| target triple = "spir64-unknown-unknown" | ||
|
|
||
| ; CHECK-SPIRV: Capability AbortKHR | ||
| ; CHECK-SPIRV: Extension "SPV_KHR_abort" | ||
| ; CHECK-SPIRV: TypeInt [[#I32Ty:]] 32 | ||
| ; CHECK-SPIRV: FunctionParameter [[#I32Ty]] [[#MsgId:]] | ||
| ; CHECK-SPIRV: AbortKHR [[#I32Ty]] [[#MsgId]] | ||
|
|
||
| ; CHECK-LLVM: call spir_func void @{{.*__spirv_AbortKHR.*}}(i32 %{{.*}}){{.*}}#[[#ATTR:]] | ||
| ; CHECK-LLVM-NEXT: unreachable | ||
| ; CHECK-LLVM: attributes #[[#ATTR]] = {{{.*}}noreturn{{.*}}} | ||
|
|
||
| define spir_func void @test_abort(i32 %msg) { | ||
| entry: | ||
| call spir_func void @_Z16__spirv_AbortKHRIiEvT_(i32 %msg) | ||
| ret void | ||
| } | ||
|
|
||
| ; Same as @test_abort, but with an explicit `unreachable` terminator instead of | ||
| ; `ret void`. Both forms must lower to a single OpAbortKHR with no trailing | ||
| ; OpUnreachable / OpReturn. | ||
| define spir_func void @test_abort_unreachable(i32 %msg) { | ||
| entry: | ||
| call spir_func void @_Z16__spirv_AbortKHRIiEvT_(i32 %msg) | ||
| unreachable | ||
| } | ||
|
|
||
| declare spir_func void @_Z16__spirv_AbortKHRIiEvT_(i32) | ||
|
|
||
| !opencl.spir.version = !{!0} | ||
| !spirv.Source = !{!1} | ||
|
|
||
| !0 = !{i32 1, i32 2} | ||
| !1 = !{i32 4, i32 100000} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we split in two paths? This one for spirv-friendly IR, and another one for non-friendly which would translate into... llvm.trap maybe?