[SPIRV] Add support for SPV_KHR_abort extension#193037
Conversation
This commit adds support for the SPV_KHR_abort extension in the SPIRV backend. The extension allows shaders to abort execution with a custom message. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>
|
@llvm/pr-subscribers-backend-spir-v @llvm/pr-subscribers-llvm-ir Author: Victor Mustya (vmustya) ChangesThis commit adds support for the SPV_KHR_abort extension in the SPIRV Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Patch is 30.57 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/193037.diff 16 Files Affected:
diff --git a/llvm/docs/SPIRVUsage.rst b/llvm/docs/SPIRVUsage.rst
index 75a477b86da05..e66f8bb374a94 100644
--- a/llvm/docs/SPIRVUsage.rst
+++ b/llvm/docs/SPIRVUsage.rst
@@ -213,6 +213,8 @@ Below is a list of supported SPIR-V extensions, sorted alphabetically by their e
- Allows to allocate local arrays whose number of elements is unknown at compile time.
* - ``SPV_INTEL_joint_matrix``
- Adds few matrix capabilities on top of SPV_KHR_cooperative_matrix extension, such as matrix prefetch, get element coordinate and checked load/store/construct instructions, tensor float 32 and bfloat type interpretations for multiply-add instruction.
+ * - ``SPV_KHR_abort``
+ - Adds a function-termination instruction that signals an abnormal error to the client API. Currently supported in OpenCL environments only.
* - ``SPV_KHR_bit_instructions``
- Enables bit instructions to be used by SPIR-V modules without requiring the Shader capability.
* - ``SPV_KHR_expect_assume``
diff --git a/llvm/include/llvm/IR/IntrinsicsSPIRV.td b/llvm/include/llvm/IR/IntrinsicsSPIRV.td
index d2a5fa1f08724..c4733d3942f2e 100644
--- a/llvm/include/llvm/IR/IntrinsicsSPIRV.td
+++ b/llvm/include/llvm/IR/IntrinsicsSPIRV.td
@@ -39,6 +39,7 @@ let TargetPrefix = "spv" in {
def int_spv_selection_merge : Intrinsic<[], [llvm_any_ty, llvm_i32_ty], [ImmArg<ArgIndex<1>>]>;
def int_spv_cmpxchg : Intrinsic<[llvm_i32_ty], [llvm_any_ty, llvm_vararg_ty]>;
def int_spv_unreachable : Intrinsic<[], []>;
+ def int_spv_abort : Intrinsic<[], [llvm_vararg_ty], [IntrNoReturn]>;
def int_spv_alloca : Intrinsic<[llvm_any_ty], [llvm_i32_ty], [ImmArg<ArgIndex<0>>]>;
def int_spv_alloca_array : Intrinsic<[llvm_any_ty], [llvm_anyint_ty, llvm_i32_ty], [ImmArg<ArgIndex<1>>]>;
def int_spv_undef : Intrinsic<[llvm_i32_ty], []>;
diff --git a/llvm/lib/Target/SPIRV/SPIRVCommandLine.cpp b/llvm/lib/Target/SPIRV/SPIRVCommandLine.cpp
index 734a03ff60141..863c3b6d1f757 100644
--- a/llvm/lib/Target/SPIRV/SPIRVCommandLine.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVCommandLine.cpp
@@ -176,7 +176,8 @@ static const StringMap<SPIRV::Extension::Extension> SPIRVExtensionMap = {
{"SPV_EXT_image_raw10_raw12",
SPIRV::Extension::Extension::SPV_EXT_image_raw10_raw12},
{"SPV_INTEL_unstructured_loop_controls",
- SPIRV::Extension::Extension::SPV_INTEL_unstructured_loop_controls}};
+ SPIRV::Extension::Extension::SPV_INTEL_unstructured_loop_controls},
+ {"SPV_KHR_abort", SPIRV::Extension::Extension::SPV_KHR_abort}};
bool SPIRVExtensionsParser::parse(cl::Option &O, StringRef ArgName,
StringRef ArgValue, ExtensionSet &Vals) {
diff --git a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
index 7a886de005b88..6e109c1dd74ab 100644
--- a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
@@ -2253,6 +2253,20 @@ Instruction *SPIRVEmitIntrinsics::visitAtomicCmpXchgInst(AtomicCmpXchgInst &I) {
Instruction *SPIRVEmitIntrinsics::visitUnreachableInst(UnreachableInst &I) {
IRBuilder<> B(I.getParent());
B.SetInsertPoint(&I);
+ // OpAbortKHR is itself a SPIR-V block terminator. If the previous instruction
+ // is a call to llvm.spv.abort, do not emit an additional OpUnreachable, which
+ // would leave the block with two terminators and produce invalid SPIR-V.
+ for (Instruction *Prev = I.getPrevNode(); Prev; Prev = Prev->getPrevNode()) {
+ if (Prev->isDebugOrPseudoInst())
+ continue;
+ auto *CI = dyn_cast<CallInst>(Prev);
+ if (!CI)
+ break;
+ Intrinsic::ID IID = CI->getIntrinsicID();
+ if (IID == Intrinsic::spv_abort)
+ return &I;
+ break;
+ }
B.CreateIntrinsic(Intrinsic::spv_unreachable, {});
return &I;
}
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
index 04ff442bbeed2..a1162f2fbc664 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
+++ b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
@@ -641,6 +641,8 @@ let isReturn = 1, hasDelaySlot = 0, isBarrier = 0, isTerminator = 1, isNotDuplic
def OpReturn: SimpleOp<"OpReturn", 253>;
def OpReturnValue: Op<254, (outs), (ins ID:$ret), "OpReturnValue $ret">;
def OpUnreachable: SimpleOp<"OpUnreachable", 255>;
+ def OpAbortKHR: Op<5121, (outs), (ins TYPE:$msg_type, ID:$msg),
+ "OpAbortKHR $msg_type $msg">;
}
def OpLifetimeStart: Op<256, (outs), (ins ID:$ptr, i32imm:$sz), "OpLifetimeStart $ptr $sz">;
def OpLifetimeStop: Op<257, (outs), (ins ID:$ptr, i32imm:$sz), "OpLifetimeStop $ptr $sz">;
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
index f1e0450bb20f9..7414880ecb05b 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
@@ -350,6 +350,7 @@ class SPIRVInstructionSelector : public InstructionSelector {
bool diagnoseUnsupported(const MachineInstr &I, const Twine &Msg) const;
+ bool selectAbort(MachineInstr &I) const;
bool selectFrameIndex(Register ResVReg, SPIRVTypeInst ResType,
MachineInstr &I) const;
bool selectAllocaArray(Register ResVReg, SPIRVTypeInst ResType,
@@ -4635,6 +4636,8 @@ bool SPIRVInstructionSelector::selectIntrinsic(Register ResVReg,
BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpUnreachable))
.constrainAllUses(TII, TRI, RBI);
return true;
+ case Intrinsic::spv_abort:
+ return selectAbort(I);
case Intrinsic::spv_alloca:
return selectFrameIndex(ResVReg, ResType, I);
case Intrinsic::spv_alloca_array:
@@ -6250,6 +6253,64 @@ bool SPIRVInstructionSelector::selectAllocaArray(Register ResVReg,
return true;
}
+bool SPIRVInstructionSelector::selectAbort(MachineInstr &I) const {
+ if (!STI.canUseExtension(SPIRV::Extension::SPV_KHR_abort))
+ report_fatal_error("OpAbortKHR instruction requires the following "
+ "SPIR-V extension: SPV_KHR_abort",
+ false);
+ // The intrinsic is declared as variadic so it can carry composite message
+ // types (vectors and structs) without LLVM IR mangling restrictions, but
+ // OpAbortKHR takes exactly one Message operand.
+ if (I.getNumExplicitOperands() != 2)
+ report_fatal_error("llvm.spv.abort must be called with exactly one "
+ "message argument",
+ false);
+ Register MsgReg = I.getOperand(1).getReg();
+ SPIRVTypeInst MsgType = GR.getSPIRVTypeForVReg(MsgReg);
+ assert(MsgType && "Message argument of llvm.spv.abort has no SPIR-V type");
+ // SPV_KHR_abort requires Message Type to be a concrete type. Per the
+ // SPIR-V "Concrete Type" definition, that means a numerical scalar
+ // (int/float), a vector, matrix, or any aggregate (array/struct)
+ // recursively containing only such types. OpTypeBool, OpTypeVoid,
+ // pointers, opaque handles and similar abstract/non-concrete types are
+ // rejected up front rather than emitting invalid SPIR-V. Validate
+ // recursively so that e.g. a struct containing a bool or pointer is also
+ // rejected.
+ SmallVector<SPIRVTypeInst, 4> Worklist{MsgType};
+ while (!Worklist.empty()) {
+ SPIRVTypeInst Ty = Worklist.pop_back_val();
+ switch (Ty->getOpcode()) {
+ case SPIRV::OpTypeInt:
+ case SPIRV::OpTypeFloat:
+ break;
+ case SPIRV::OpTypeVector:
+ case SPIRV::OpTypeMatrix:
+ case SPIRV::OpTypeArray:
+ // Operand 1 holds the element/component type id.
+ Worklist.push_back(
+ GR.getSPIRVTypeForVReg(Ty->getOperand(1).getReg()));
+ break;
+ case SPIRV::OpTypeStruct:
+ // Operands 1..N hold the field type ids.
+ for (unsigned Idx = 1, E = Ty->getNumOperands(); Idx < E; ++Idx)
+ Worklist.push_back(
+ GR.getSPIRVTypeForVReg(Ty->getOperand(Idx).getReg()));
+ break;
+ default:
+ report_fatal_error("llvm.spv.abort message type must be a concrete "
+ "SPIR-V type (numerical scalar, vector, matrix, "
+ "or aggregate of such types)",
+ false);
+ }
+ }
+ MachineBasicBlock &BB = *I.getParent();
+ BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpAbortKHR))
+ .addUse(GR.getSPIRVTypeID(MsgType))
+ .addUse(MsgReg)
+ .constrainAllUses(TII, TRI, RBI);
+ return true;
+}
+
bool SPIRVInstructionSelector::selectFrameIndex(Register ResVReg,
SPIRVTypeInst ResType,
MachineInstr &I) const {
diff --git a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
index aec9b15df9189..5412e50d6afc5 100644
--- a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
@@ -1945,6 +1945,14 @@ void addInstrRequirements(const MachineInstr &MI,
Reqs.addExtension(SPIRV::Extension::SPV_KHR_shader_clock);
Reqs.addCapability(SPIRV::Capability::ShaderClockKHR);
break;
+ case SPIRV::OpAbortKHR:
+ if (!ST.canUseExtension(SPIRV::Extension::SPV_KHR_abort))
+ report_fatal_error("OpAbortKHR instruction requires the "
+ "following SPIR-V extension: SPV_KHR_abort",
+ false);
+ Reqs.addExtension(SPIRV::Extension::SPV_KHR_abort);
+ Reqs.addCapability(SPIRV::Capability::AbortKHR);
+ break;
case SPIRV::OpFunctionPointerCallINTEL:
if (ST.canUseExtension(SPIRV::Extension::SPV_INTEL_function_pointers)) {
Reqs.addExtension(SPIRV::Extension::SPV_INTEL_function_pointers);
diff --git a/llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp b/llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp
index a3b44ad6d31d5..7e955e2d6d2df 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp
@@ -19,6 +19,7 @@
//===----------------------------------------------------------------------===//
#include "SPIRV.h"
+#include "SPIRVBuiltins.h"
#include "SPIRVSubtarget.h"
#include "SPIRVTargetMachine.h"
#include "SPIRVUtils.h"
@@ -33,6 +34,7 @@
#include "llvm/IR/Intrinsics.h"
#include "llvm/IR/IntrinsicsSPIRV.h"
#include "llvm/Transforms/Utils/Cloning.h"
+#include "llvm/Transforms/Utils/Local.h"
#include "llvm/Transforms/Utils/LowerMemIntrinsics.h"
#include <regex>
@@ -43,6 +45,7 @@ namespace {
class SPIRVPrepareFunctions : public ModulePass {
const SPIRVTargetMachine &TM;
bool substituteIntrinsicCalls(Function *F);
+ bool substituteAbortKHRCalls(Function *F);
Function *removeAggregateTypesFromSignature(Function *F);
bool removeAggregateTypesFromCalls(Function *F);
@@ -595,6 +598,62 @@ SPIRVPrepareFunctions::removeAggregateTypesFromSignature(Function *F) {
return NewF;
}
+// Replace OpenCL/SPIR-V style calls to `__spirv_AbortKHR(message)` with calls
+// to the `llvm.spv.abort` target intrinsic, so that they go through the same
+// instruction-selection path as the intrinsic and get lowered to OpAbortKHR.
+bool SPIRVPrepareFunctions::substituteAbortKHRCalls(Function *F) {
+ if (F->isDeclaration())
+ return false;
+
+ SmallVector<CallInst *> Calls;
+ for (Instruction &I : instructions(F)) {
+ auto *CI = dyn_cast<CallInst>(&I);
+ if (!CI)
+ continue;
+ Function *Callee = CI->getCalledFunction();
+ if (!Callee || Callee->isIntrinsic())
+ continue;
+ StringRef Demangled = Callee->getName();
+ std::string DemangledStr = getOclOrSpirvBuiltinDemangledName(Demangled);
+ if (DemangledStr.empty())
+ continue;
+ std::string BuiltinName = SPIRV::lookupBuiltinNameHelper(DemangledStr);
+ if (StringRef(BuiltinName) != "__spirv_AbortKHR")
+ continue;
+ if (CI->arg_size() != 1)
+ continue;
+ Calls.push_back(CI);
+ }
+
+ if (Calls.empty())
+ return false;
+
+ for (CallInst *CI : Calls) {
+ IRBuilder<> B(CI);
+ Value *Msg = CI->getArgOperand(0);
+ // The OpenCL C ABI may pass aggregate arguments by pointer (byval). In
+ // that case load the underlying value so that OpAbortKHR receives the
+ // composite itself, as required by the SPV_KHR_abort spec ("Message Type
+ // must be a concrete type").
+ if (CI->isByValArgument(0)) {
+ Type *AggTy = CI->getParamByValType(0);
+ Msg = B.CreateLoad(AggTy, Msg);
+ }
+ B.CreateIntrinsic(Intrinsic::spv_abort, {}, {Msg});
+ // OpAbortKHR is itself a SPIR-V function-termination instruction and must
+ // be the last instruction in its block. Drop the original call and
+ // everything that follows it (the OpenCL ABI typically appends stores into
+ // the return slot and a `ret`), and re-terminate the block with
+ // `unreachable`. We use changeToUnreachable so that any successor PHI
+ // nodes have the now-removed predecessor edge cleaned up; otherwise the
+ // IR verifier would reject mismatched PHI incoming entries. The matching
+ // suppression in SPIRVEmitIntrinsics::visitUnreachableInst ensures no
+ // extra OpUnreachable is emitted after OpAbortKHR.
+ changeToUnreachable(CI);
+ }
+ return true;
+}
+
// Mutates indirect callsites iff if aggregate argument/return types are present
// with the types replaced by i32 types. The change in types is noted in
// 'spv.mutated_callsites' metadata for later restoration.
@@ -684,6 +743,7 @@ bool SPIRVPrepareFunctions::runOnModule(Module &M) {
for (Function &F : M) {
Changed |= substituteIntrinsicCalls(&F);
+ Changed |= substituteAbortKHRCalls(&F);
Changed |= sortBlocks(F);
Changed |= removeAggregateTypesFromCalls(&F);
}
diff --git a/llvm/lib/Target/SPIRV/SPIRVSymbolicOperands.td b/llvm/lib/Target/SPIRV/SPIRVSymbolicOperands.td
index 56608a80f4b23..68ee90f2c6792 100644
--- a/llvm/lib/Target/SPIRV/SPIRVSymbolicOperands.td
+++ b/llvm/lib/Target/SPIRV/SPIRVSymbolicOperands.td
@@ -398,6 +398,10 @@ defm SPV_EXT_image_raw10_raw12 :ExtensionOperand<133, [EnvOpenCL, EnvVulkan]>;
defm SPV_ALTERA_arbitrary_precision_floating_point: ExtensionOperand<134, [EnvOpenCL]>;
defm SPV_KHR_fma : ExtensionOperand<135, [EnvVulkan, EnvOpenCL]>;
defm SPV_INTEL_masked_gather_scatter : ExtensionOperand<136, [EnvOpenCL]>;
+// SPV_KHR_abort is currently only supported in the OpenCL environment.
+// Vulkan/Shader support requires emitting explicit layout decorations on the
+// Message Type, which is not yet implemented.
+defm SPV_KHR_abort : ExtensionOperand<137, [EnvOpenCL]>;
//===----------------------------------------------------------------------===//
// Multiclass used to define Capabilities enum values and at the same time
@@ -598,6 +602,7 @@ defm GlobalVariableFPGADecorationsINTEL : CapabilityOperand<6189, 0, 0, [SPV_INT
defm CacheControlsINTEL : CapabilityOperand<6441, 0, 0, [SPV_INTEL_cache_controls], []>;
defm CooperativeMatrixKHR : CapabilityOperand<6022, 0, 0, [SPV_KHR_cooperative_matrix], []>;
defm ArithmeticFenceEXT : CapabilityOperand<6144, 0, 0, [SPV_EXT_arithmetic_fence], []>;
+defm AbortKHR : CapabilityOperand<5120, 0, 0, [SPV_KHR_abort], []>;
defm SplitBarrierINTEL : CapabilityOperand<6141, 0, 0, [SPV_INTEL_split_barrier], []>;
defm CooperativeMatrixCheckedInstructionsINTEL : CapabilityOperand<6192, 0, 0, [SPV_INTEL_joint_matrix], []>;
defm CooperativeMatrixPrefetchINTEL : CapabilityOperand<6411, 0, 0, [SPV_INTEL_joint_matrix], []>;
diff --git a/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_abort/abort-composite-construct.ll b/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_abort/abort-composite-construct.ll
new file mode 100644
index 0000000000000..b7ab99b47b0cd
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_abort/abort-composite-construct.ll
@@ -0,0 +1,30 @@
+; RUN: llc -O0 -mtriple=spirv32-unknown-unknown --spirv-ext=+SPV_KHR_abort %s -o - | FileCheck %s
+; RUN: llc -O0 -mtriple=spirv64-unknown-unknown --spirv-ext=+SPV_KHR_abort %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown --spirv-ext=+SPV_KHR_abort %s -o - -filetype=obj | spirv-val %}
+
+;; Verify that OpAbortKHR can consume a composite produced by
+;; OpCompositeConstruct, mirroring the SPV_KHR_abort spec example which builds
+;; the message via OpCompositeConstruct.
+
+; CHECK-DAG: OpCapability AbortKHR
+; CHECK-DAG: OpExtension "SPV_KHR_abort"
+; CHECK-DAG: %[[#I32:]] = OpTypeInt 32 0
+; CHECK-DAG: %[[#V2:]] = OpTypeVector %[[#I32]] 2
+
+; CHECK: %[[#CC:]] = OpCompositeConstruct %[[#V2]] %{{[0-9]+}} %{{[0-9]+}}
+; CHECK: OpAbortKHR %[[#V2]] %[[#CC]]
+; CHECK-NOT: OpUnreachable
+
+declare void @llvm.spv.abort(...) #0
+
+define spir_kernel void @abort_composite_construct(i32 %a, i32 %b) {
+entry:
+ %va = insertelement <1 x i32> poison, i32 %a, i32 0
+ %vb = insertelement <1 x i32> poison, i32 %b, i32 0
+ %v = shufflevector <1 x i32> %va, <1 x i32> %vb,
+ <2 x i32> <i32 0, i32 1>
+ call void (...) @llvm.spv.abort(<2 x i32> %v)
+ unreachable
+}
+
+attributes #0 = { noreturn }
diff --git a/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_abort/abort-composite.ll b/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_abort/abort-composite.ll
new file mode 100644
index 0000000000000..f8c779dc7b891
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_abort/abort-composite.ll
@@ -0,0 +1,51 @@
+; RUN: llc -O0 -mtriple=spirv32-unknown-unknown --spirv-ext=+SPV_KHR_abort %s -o - | FileCheck %s
+; RUN: llc -O0 -mtriple=spirv64-unknown-unknown --spirv-ext=+SPV_KHR_abort %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown --spirv-ext=+SPV_KHR_abort %s -o - -filetype=obj | spirv-val %}
+
+; Verify that OpAbortKHR can consume a composite (struct or vector) message,
+; built up via OpCompositeInsert, mirroring the example in the SPV_KHR_abort
+; specification.
+
+; CHECK-DAG: OpCapability AbortKHR
+; CHECK-DAG: OpExtension "SPV_KHR_abort"
+
+; CHECK-DAG: %[[#I32:]] = OpTypeInt 32 0
+; CHECK-DAG: %[[#F32:]] = OpTypeFloat 32
+; CHECK-DAG: %[[#STRUCT:]] = OpTypeStruct %[[#I32]] %[[#I32]] %[[#F32]]
+; CHECK-DAG: %[[#VEC:]] = OpTypeVector %[[#I32]] 4
+
+; CHECK: %[[#S0:]] = OpCompositeInsert %[[#STRUCT]] %{{[0-9]+}} %{{[0-9]+}} 0
+; CHECK: %[[#S1:]] = OpCompositeInsert %[[#STRUCT]] %{{[0-9]+}} %[[#S0]] 1
+; CHECK: %[[#S2:]] = OpCompositeInsert %[[#STRUCT]] %{{[0-9]+}} %[[#S1]] 2
+; CHECK: OpAbortKHR %[[#STRUCT]] %[[#S2]]
+
+; CHECK: %[[#V0:]] = OpCompositeInsert %[[#VEC]] %{{[0-9]+}} %{{[0-9]+}} 0
+; CHECK: %[[#V1:]] = OpCompositeInsert %[[#VEC]] %{{[0-9]+}} %[[#V0]] 1
+; CHECK: %[[#V2:]] = OpCompositeInsert %[[#VEC]] %{{[0-9]+}} %[[#V1]] 2
+; CHECK: %[[#V3:]] = OpCompositeInsert %[[#VEC]] %{{[0-9]+}} %[[#V2]] 3
+; CHECK: OpAbortKHR %[[#VEC]] %[[#V3]]
+
+%struct.Msg = type { i32, i32, float }
+
+declare void @llvm.spv.abort(...) #0
+
+define spir_kernel void @abort_with_struct(i32 %x, i32 %y, float %z) {
+entry:
+ %m0 = insertvalue %struct.Msg poison, i32 %x, 0
+ %m1 = insertvalue %struct.Msg %m0, i32 %y, 1
+ %m2 = insertvalue %struct.Msg %m1, float %z, 2
+ call void (...) @llvm.spv.abort(%struct.Msg %m2)
+ unreachable
+}
+
+define spir_kernel void @abort_with_vector(i32 %a, i32 %b, i32 %c, i32 %d) {
+entry:
+ %v0 = insertelement <4 x i32> poison, i32 %a, i32 0
+ %v1 = insertelement <4 x i32> %v0, i32 %b, i32 1
+ %v2 = insertelement <4 x i32> %v1, i32 %c, i32 2
+ %v3 = insertelement <4 x i32> %v2, i32 %d, i32 3
+ call void (...) @llvm.spv.abort(<4 x i32> %v3)
+ unreachable
+}
+
+attributes #0 = { noreturn }
diff --git a/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_abort/abort-invalid-message-type.ll b/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_abort/abort-invalid-message-type.ll
new file mode 100644
index 0000000000000..d491ac70d80de
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_abort/abort-invalid-message-type.ll
@@ -0,0 +1,61 @@
+; RUN: split-file %s %t
+; RUN: not llc -O0 -mtriple=spirv64-unknown-unknown --spirv-ext=+SPV_KHR_abort %t/top-level.ll -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llc -O0 -mtriple=spirv64-unknown-unknown --spirv-ext=+SPV_KHR_abort %t/nested-struct.ll -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llc -O0 -mtriple=spirv64-unknown-unknown --spirv-ext=+SPV_KHR_abort %t/nested-array.ll -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llc -O0 -mtriple=spirv64-unknown-unknown --spirv-ext=+SPV_KHR_abort %t/top-level-bool.ll -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llc -...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| @@ -6250,6 +6253,63 @@ bool SPIRVInstructionSelector::selectAllocaArray(Register ResVReg, | |||
| return true; | |||
| } | |||
|
|
|||
| bool SPIRVInstructionSelector::selectAbort(MachineInstr &I) const { | |||
| if (!STI.canUseExtension(SPIRV::Extension::SPV_KHR_abort)) | |||
There was a problem hiding this comment.
Should we also check for/enable implicitly SPV_KHR_constant_data or the ConstantDataKHR (not necessarily here)? I think ConstantDataKHR has not been added to the SPIR-V backend
There was a problem hiding this comment.
The extension formally requires the SPV_KHR_constand_data, but there are no usage requirements or validation rules that would make the constant data extension mandatory. Let's keep the implementation as-is for now, and clarify the requirements with the SPV_KHR_abort extension authors.
| while (!Worklist.empty()) { | ||
| SPIRVTypeInst Ty = Worklist.pop_back_val(); | ||
| switch (Ty->getOpcode()) { | ||
| case SPIRV::OpTypeInt: | ||
| case SPIRV::OpTypeFloat: | ||
| break; | ||
| case SPIRV::OpTypeVector: | ||
| case SPIRV::OpTypeMatrix: | ||
| case SPIRV::OpTypeArray: | ||
| // Operand 1 holds the element/component type id. | ||
| Worklist.push_back(GR.getSPIRVTypeForVReg(Ty->getOperand(1).getReg())); | ||
| break; | ||
| case SPIRV::OpTypeStruct: | ||
| // Operands 1..N hold the field type ids. | ||
| for (unsigned Idx = 1, E = Ty->getNumOperands(); Idx < E; ++Idx) | ||
| Worklist.push_back( | ||
| GR.getSPIRVTypeForVReg(Ty->getOperand(Idx).getReg())); | ||
| break; | ||
| default: |
There was a problem hiding this comment.
Could this also be a physical pointer (since it can be any concrete type)?
| Intrinsic::ID IID = CI->getIntrinsicID(); | ||
| if (IID == Intrinsic::spv_abort) | ||
| return &I; | ||
| break; |
There was a problem hiding this comment.
I think this will emit spv_unreachable if there is any real instruction between spv_abort and unreachable. Not sure if such situation can happen in reality, but we could add an assert.
E.g.:
call @llvm.spv.abort(...)
%tmp = freeze i32 poison
unreachable
…ollowed by unreachable
| #ifndef NDEBUG | ||
| for (Instruction *P = I.getPrevNode(); P; P = P->getPrevNode()) { | ||
| auto *CI = dyn_cast<CallInst>(P); | ||
| if (CI && CI->getIntrinsicID() == Intrinsic::spv_abort) | ||
| llvm_unreachable("llvm.spv.abort must be the last non-debug instruction " | ||
| "before its block's `unreachable`; OpAbortKHR is itself " | ||
| "a SPIR-V block terminator"); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
This kind of check is for the verifier and should not be in here. You should be able to assume that the IR is well formed.
Or if it's only meant for debug builds, like an assertion, then use an actual assertion.
PS: I'll do the later.
On a totally different aspect, instead of iteration backwards, you can iterate from the beginning of the block until I and make the code easier to read.
There was a problem hiding this comment.
Also, I'm not sure the check is correct. Look at the example I posted in the translator PR: KhronosGroup/SPIRV-LLVM-Translator#3691 (comment)
Why would that be invalid? I think we should be able to gracefully handle that instead of asserting.
There was a problem hiding this comment.
I've implemented the handling on the stage, when the __spirv_AbortKHR built-in function is translated to the llvm.spv.abort intrinsic. Also, I've re-written the check to an assertion: 6bacaf1
| report_fatal_error("OpAbortKHR instruction requires the following " | ||
| "SPIR-V extension: SPV_KHR_abort", | ||
| false); | ||
| // The intrinsic is declared as variadic so it can carry composite message |
There was a problem hiding this comment.
This shouldn't hit a fatal error. This should call SPIRVInstructionSelector::diagnoseUnsupported and return false to trigger a failure to select.
| if (I.getNumExplicitOperands() != 2) | ||
| report_fatal_error("llvm.spv.abort must be called with exactly one " | ||
| "message argument", | ||
| false); |
There was a problem hiding this comment.
This kind of check should never happen in well-formed LLVM-IR right?
You could do it in the verifier. Or put an assert(I.getNumExplicitOperands() == 2); for the developers.
| @@ -6250,6 +6253,63 @@ bool SPIRVInstructionSelector::selectAllocaArray(Register ResVReg, | |||
| return true; | |||
| } | |||
|
|
|||
| bool SPIRVInstructionSelector::selectAbort(MachineInstr &I) const { | |||
| if (!STI.canUseExtension(SPIRV::Extension::SPV_KHR_abort)) | |||
There was a problem hiding this comment.
I don't think you should be doing this check in here. It is done already in llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp. But I get that we're inconsistent with this on this file.
There was a problem hiding this comment.
There are similar checks already implemented for the selectStackSave and selectStackRestore functions, so I'd keep the check here. I've rewritten it from the fatal error to the diagnoseUnsupported.
| for (Instruction &I : instructions(F)) { | ||
| auto *CI = dyn_cast<CallInst>(&I); | ||
| if (!CI) | ||
| continue; | ||
| Function *Callee = CI->getCalledFunction(); | ||
| if (!Callee || Callee->isIntrinsic()) | ||
| continue; | ||
| StringRef Demangled = Callee->getName(); | ||
| std::string DemangledStr = getOclOrSpirvBuiltinDemangledName(Demangled); | ||
| if (DemangledStr.empty()) | ||
| continue; | ||
| std::string BuiltinName = SPIRV::lookupBuiltinNameHelper(DemangledStr); | ||
| if (StringRef(BuiltinName) != "__spirv_AbortKHR") | ||
| continue; | ||
| if (CI->arg_size() != 1) | ||
| continue; | ||
| Calls.push_back(CI); | ||
| } |
There was a problem hiding this comment.
You should do this the other way around, scan the functions in the module looking for __spirv_AbortKHR; then do the replacement on its Users.
Otherwise we scan the whole module even if __spirv_AbortKHR is not used.
| @@ -39,6 +39,7 @@ let TargetPrefix = "spv" in { | |||
| def int_spv_selection_merge : Intrinsic<[], [llvm_any_ty, llvm_i32_ty], [ImmArg<ArgIndex<1>>]>; | |||
| def int_spv_cmpxchg : Intrinsic<[llvm_i32_ty], [llvm_any_ty, llvm_vararg_ty]>; | |||
| def int_spv_unreachable : Intrinsic<[], []>; | |||
| def int_spv_abort : Intrinsic<[], [llvm_vararg_ty], [IntrNoReturn]>; | |||
There was a problem hiding this comment.
What if we parametrize the intrinsic by type, instead of taking a vararg?
There was a problem hiding this comment.
I do believe that we need to implement constant data extension before abort so we could use constant data in the arg.
There was a problem hiding this comment.
On a second thought, not sure I agree. If instead of a constant string we would like to pass simply a constant int as message, I think that would be valid for OpAbortKHR, wouldn't it? So the constant data extension wouldn't be required for that, would it? Also, I'm concerned about the fact that this accepts any number of arguments, while OpAbortKHR only accepts one: the message --the type of that one should be implicit. Should we use llvm_any_ty instead?
There was a problem hiding this comment.
I withdraw my comment requiring a constant data. Lets have some implementation experience to may be provide feedback to the specification.
There was a problem hiding this comment.
I've converted the intrinsic operand type from the vararg to the llvm_any_ty: 6bacaf1
| return false; | ||
|
|
||
| SmallVector<CallInst *> Calls; | ||
| for (Instruction &I : instructions(F)) { |
There was a problem hiding this comment.
I think calls to @llvm.trap should also translate to a call to spv_abort (if and only if the extension is available). Currently a call to @llvm.trap leads to a no-op that is against the langref specification.
There was a problem hiding this comment.
+1 to this. We can leave it as todo for now, but ultimately we should be lowering llvm.trap to spirv.abort. The Q would be: "what kind of message we should pass in this case?", may be something like: "abort is caused by llvm.trap with no debug string" is good enough.
There was a problem hiding this comment.
I would like the translation to be implemented in this very patch, if possible. It's tightly related, so we may as well have it all together. Regarding the message, we could try and be a little more verbose and pass maybe something more meaningful. I'd say maybe a combination of the instruction, basic block, function and module, so that at least it's possible to know where the abort happened. I'd be happy to have that in a follow up, though. Something basic is good to start with.
There was a problem hiding this comment.
I'd prefer to emit something like OpAbortKHR Int32 0 in this case, and let the device compiler to generate its own message. @jmmartinez, @MrSidims, @maarquitos14, what do you think?
There was a problem hiding this comment.
That works for me as a starting point. We can follow up in the future.
There was a problem hiding this comment.
Sounds good to me. If anything, we can change it to something else later.
PS: I'll be on vacations so I won't be able to go further with the review. But don't wait for my approval to land this change.
There was a problem hiding this comment.
Implemented both llvm.trap and llvm.ubsantrap as follows:
- The
llvm.ubsantrapis lowered intoOpAbortKHRwith theubsantrapoperand zero-extended to 32 bits and used as the abort "message". - For the
llvm.trapmessage, the 32-bit constant0xFFFFFFFFis used.
See the details: 6bacaf1
| // rejected up front rather than emitting invalid SPIR-V. Validate | ||
| // recursively so that e.g. a struct containing a bool is also rejected. | ||
| SmallVector<SPIRVTypeInst, 4> Worklist{MsgType}; | ||
| while (!Worklist.empty()) { |
There was a problem hiding this comment.
I think this deserves its own function.
|
This PR may be related to #178983 and could be a way to implement the support for |
| @@ -39,6 +39,7 @@ let TargetPrefix = "spv" in { | |||
| def int_spv_selection_merge : Intrinsic<[], [llvm_any_ty, llvm_i32_ty], [ImmArg<ArgIndex<1>>]>; | |||
| def int_spv_cmpxchg : Intrinsic<[llvm_i32_ty], [llvm_any_ty, llvm_vararg_ty]>; | |||
| def int_spv_unreachable : Intrinsic<[], []>; | |||
| def int_spv_abort : Intrinsic<[], [llvm_vararg_ty], [IntrNoReturn]>; | |||
There was a problem hiding this comment.
On a second thought, not sure I agree. If instead of a constant string we would like to pass simply a constant int as message, I think that would be valid for OpAbortKHR, wouldn't it? So the constant data extension wouldn't be required for that, would it? Also, I'm concerned about the fact that this accepts any number of arguments, while OpAbortKHR only accepts one: the message --the type of that one should be implicit. Should we use llvm_any_ty instead?
| #ifndef NDEBUG | ||
| for (Instruction *P = I.getPrevNode(); P; P = P->getPrevNode()) { | ||
| auto *CI = dyn_cast<CallInst>(P); | ||
| if (CI && CI->getIntrinsicID() == Intrinsic::spv_abort) | ||
| llvm_unreachable("llvm.spv.abort must be the last non-debug instruction " | ||
| "before its block's `unreachable`; OpAbortKHR is itself " | ||
| "a SPIR-V block terminator"); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Also, I'm not sure the check is correct. Look at the example I posted in the translator PR: KhronosGroup/SPIRV-LLVM-Translator#3691 (comment)
Why would that be invalid? I think we should be able to gracefully handle that instead of asserting.
| report_fatal_error("OpAbortKHR instruction requires the following " | ||
| "SPIR-V extension: SPV_KHR_abort", | ||
| false); | ||
| // The intrinsic is declared as variadic so it can carry composite message |
There was a problem hiding this comment.
Maybe the problem is that it was declared as variadic in the first place. Why do we need that?
| return false; | ||
|
|
||
| SmallVector<CallInst *> Calls; | ||
| for (Instruction &I : instructions(F)) { |
There was a problem hiding this comment.
I would like the translation to be implemented in this very patch, if possible. It's tightly related, so we may as well have it all together. Regarding the message, we could try and be a little more verbose and pass maybe something more meaningful. I'd say maybe a combination of the instruction, basic block, function and module, so that at least it's possible to know where the abort happened. I'd be happy to have that in a follow up, though. Something basic is good to start with.
# Conflicts: # llvm/lib/Target/SPIRV/SPIRVCommandLine.cpp # llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp # llvm/lib/Target/SPIRV/SPIRVSymbolicOperands.td
…sics; add more tests
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
maarquitos14
left a comment
maarquitos14
left a comment
There was a problem hiding this comment.
LGTM, just a few nits.
Co-authored-by: Marcos Maronas <mmaronas@amd.com>
This commit adds support for the SPV_KHR_abort extension in the SPIRV backend. The extension allows shaders to abort execution with a custom message. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Marcos Maronas <mmaronas@amd.com>
This commit adds support for the SPV_KHR_abort extension in the SPIRV backend. The extension allows shaders to abort execution with a custom message. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Marcos Maronas <mmaronas@amd.com>
7 participants
This commit adds support for the SPV_KHR_abort extension in the SPIRV
backend. The extension allows shaders to abort execution with a custom
message.
Assisted-by: Claude Opus 4.7 noreply@anthropic.com