Implement SPV_KHR_abort extension by vmustya · Pull Request #3691 · KhronosGroup/SPIRV-LLVM-Translator

vmustya · 2026-04-20T17:16:40Z

The SPV_KHR_abort extension introduces the OpAbortKHR instruction,
allowing shaders to terminate execution early.

Assisted-by: Claude Opus 4.7 noreply@anthropic.com

The SPV_KHR_abort extension introduces the `OpAbortKHR` instruction, allowing shaders to terminate execution early. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>

CLAassistant · 2026-04-20T17:16:46Z

All committers have signed the CLA.

michalpaszkowski

LGTM from my side!

vmaksimo

SPV_KHR_constant_data is listed as a dependency, but I don't see it used anywhere in the code - maybe spirv-val will catch this as well.
Please also address clang-tidy failure

vmaksimo · 2026-04-21T13:01:50Z

can we have spirv-val RUN lines here?

also, please add a test case where unreachable path is also tested:

define spir_func void @test_abort_unreachable(i32 %msg) { entry: call spir_func void @_Z16__spirv_AbortKHRIiEvT_(i32 %msg) unreachable }

Done: 0b02b53

can we have spirv-val RUN lines here?

@vmaksimo , the spirv-val tool used for the CI is too old to recognize the SPV_KHR_abort extension. However, locally-built top of trunk version works correctly. Should we disable the spirv-val checks until the CI is updated?

Ouch, I was afraid of this.
Yes please, you may put smth like RUN: not spirv-val ; TODO: remove "not" once SPIR-V tools are updated so the test will start to fail in CI once tools get updated.

vmaksimo · 2026-04-21T15:49:12Z

+    if (auto *Last = BB->getTerminateInstr())
+      if (Last->getOpCode() == OpAbortKHR)
+        return mapValue(V, const_cast<SPIRVInstruction *>(Last));


if a non-void function has call @__spirv_AbortKHR the return value is silently dropped.
maybe this needs to be moved after if (auto *RV = RI->getReturnValue()) block? not sure though what LLVM semantics is expected here

Done: 0b02b53

vmustya · 2026-04-22T15:26:50Z

Please also address clang-tidy failure

The clang-tidy fails as follows:

/home/runner/work/SPIRV-LLVM-Translator/SPIRV-LLVM-Translator/lib/SPIRV/libSPIRV/SPIRVInstruction.h:903:3: error: visibility of function 'decode' is changed from public in class 'SPIRVEntry' to protected [misc-override-with-different-visibility,-warnings-as-errors]
  903 |   _SPIRV_DEF_ENCDEC2(MessageTypeId, MessageId)
      |   ^
/home/runner/work/SPIRV-LLVM-Translator/SPIRV-LLVM-Translator/lib/SPIRV/libSPIRV/SPIRVEntry.h:151:8: note: expanded from macro '_SPIRV_DEF_ENCDEC2'
  151 |   void decode(std::istream &I) override { getDecoder(I) >> (x) >> (y); }
      |        ^
/home/runner/work/SPIRV-LLVM-Translator/SPIRV-LLVM-Translator/lib/SPIRV/libSPIRV/SPIRVEntry.h:402:16: note: function declared here as public
  402 |   virtual void decode(std::istream &I);
      |                ^

The reported issue is that the encode and decode methods generated by the _SPIRV_DEF_ENCDEC2 macro are protected. However, the base SPIRVEntry class defines these methods as public. I've checked other instruction implementations, like OpReturn and OpUnreachable, and they also define the encode and decode methods as protected. So, this clang-tidy issue is applicable to the whole translator code.

I don't really want to deviate the style other instructions are implemented. So, I believe, that either all the instructions should be changed by a separate commit, or the clang-tidy error should be suppressed. @vmaksimo, what would you suggest?

maarquitos14

I was looking at SPV_KHR_abort separately, but @MrSidims brought this PR to my attention. I have a few extra tests that I'm more than happy to share if you're interested @vmustya.

maarquitos14 · 2026-04-24T09:27:09Z

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


Could we do this in this patch too? Seems very related.

@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?

maarquitos14 · 2026-04-24T09:27:24Z

+public:
+  static const Op OC = OpAbortKHR;
+  static const SPIRVWord FixedWordCount = 3;
+  // Complete constructor


Suggested change

// Complete constructor

// Complete constructor.

maarquitos14 · 2026-04-24T09:27:28Z

+    validate();
+    assert(TheBB && "Invalid BB");
+  }
+  // Incomplete constructor


Suggested change

// Incomplete constructor

// Incomplete constructor.

maarquitos14 · 2026-04-24T09:53:50Z

+  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));


I am not sure this is enough to handle all the cases. For example:

call void @llvm.trap()/__spirv_AbortKHR cal void @llvm.lifetime.end() ret void

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?

maarquitos14 · 2026-04-24T09:55:38Z

  }

+  case OpAbortKHR: {
+    // OpAbortKHR is a SPIR-V block terminator. In LLVM IR, model it as a call


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?

vmustya · 2026-04-24T12:59:10Z

I have a few extra tests that I'm more than happy to share if you're interested @vmustya.

@maarquitos14, sure! I'd really appreciate that.

maarquitos14 · 2026-04-24T15:16:59Z

I have a few extra tests that I'm more than happy to share if you're interested @vmustya.

@maarquitos14, sure! I'd really appreciate that.

You can find them in maarquitos14@fe51642. I think all of them assume llvm.trap gets translated to OpAbortKHR and back, but still I think they can be useful.

vmaksimo · 2026-04-24T16:14:10Z

I don't really want to deviate the style other instructions are implemented. So, I believe, that either all the instructions should be changed by a separate commit, or the clang-tidy error should be suppressed.

@vmustya If can - we should not introduce new errors, and I believe this is the case. So please just put encode/decode to public - we'll take a look at other instructions separately.

Implement SPV_KHR_abort extension

d209681

The SPV_KHR_abort extension introduces the `OpAbortKHR` instruction, allowing shaders to terminate execution early. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>

Fix dyn_cast -> isa

9493cdf

michalpaszkowski approved these changes Apr 21, 2026

View reviewed changes

michalpaszkowski requested a review from vmaksimo April 21, 2026 13:43

vmaksimo reviewed Apr 21, 2026

View reviewed changes

vmustya added 2 commits April 22, 2026 09:34

Address review comments

0b02b53

Restore the getOperands() method

f541009

maarquitos14 reviewed Apr 24, 2026

View reviewed changes

maarquitos14 mentioned this pull request Apr 24, 2026

[SPIRV] Add support for SPV_KHR_abort extension llvm/llvm-project#193037

Open

Conversation

vmustya commented Apr 20, 2026

Uh oh!

CLAassistant commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michalpaszkowski left a comment

Choose a reason for hiding this comment

Uh oh!

vmaksimo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vmustya commented Apr 22, 2026

Uh oh!

maarquitos14 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmustya commented Apr 24, 2026

Uh oh!

maarquitos14 commented Apr 24, 2026

Uh oh!

vmaksimo commented Apr 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Apr 20, 2026 •

edited

Loading

vmaksimo left a comment •

edited

Loading