diff --git a/examples/VulkanTriangle/README.md b/examples/VulkanTriangle/README.md index 5ff2ab5..45f9fd7 100644 --- a/examples/VulkanTriangle/README.md +++ b/examples/VulkanTriangle/README.md @@ -44,12 +44,16 @@ bug (full investigation in #7, summarised below). proprietary driver only, `VulkanShader` rewrites the compiled SPIR-V at module-load time so that every `OpLoad` of an `accelerationStructureEXT` out of the heap becomes a load of the TLAS *device address* (from a -synthesized push-constant block) followed by +push-constant block) followed by `OpConvertUToAccelerationStructureKHR` — which reads no descriptor and so never touches the faulting path. `RTPass` feeds the active frame's TLAS -address in as push data. `raygen.glsl` and the example code are unchanged; -acceleration structures still bind into the heap normally. On every other -driver the workaround is inert. It's gated on +address in as push data. SPIR-V allows only one push-constant block per +entry point, so when a shader already declares one the TLAS address is +appended to *that* block (rather than adding a second, which would fail +validation — issue #18); shaders without a push constant get a freshly +synthesized single-member block. `raygen.glsl` and the example code are +unchanged; acceleration structures still bind into the heap normally. On +every other driver the workaround is inert. It's gated on `Device::workaroundDescriptorHeapAS` and confined to one fenced block in `interfaces/Crafter.Graphics-ShaderVulkan.cppm` so it can be deleted wholesale once a fixed NVIDIA driver ships. diff --git a/interfaces/Crafter.Graphics-Device.cppm b/interfaces/Crafter.Graphics-Device.cppm index 679a40c..ed331ff 100644 --- a/interfaces/Crafter.Graphics-Device.cppm +++ b/interfaces/Crafter.Graphics-Device.cppm @@ -178,6 +178,12 @@ export namespace Crafter { // path and RTPass pushes the active TLAS address as push data. Delete // this flag and everything keyed on it once a fixed driver ships. inline static bool workaroundDescriptorHeapAS = false; + // Byte offset of the TLAS-address member inside the patched raygen's + // push-constant block — 0 for a freshly synthesized block, or the end + // of the user's own block when the address is appended to it (the + // shader can't have two push-constant blocks). VulkanShader sets this + // at module load; RTPass feeds it to vkCmdPushDataEXT. + inline static std::uint32_t workaroundTlasPushOffset = 0; static void CheckVkResult(VkResult result); static std::uint32_t GetMemoryType(std::uint32_t typeBits, VkMemoryPropertyFlags properties); diff --git a/interfaces/Crafter.Graphics-RTPass.cppm b/interfaces/Crafter.Graphics-RTPass.cppm index 65907d6..d436387 100644 --- a/interfaces/Crafter.Graphics-RTPass.cppm +++ b/interfaces/Crafter.Graphics-RTPass.cppm @@ -46,7 +46,10 @@ export namespace Crafter { VkDeviceAddress tlasAddr = RenderingElement3D::tlases[frameIdx].address; VkPushDataInfoEXT pushInfo { .sType = VK_STRUCTURE_TYPE_PUSH_DATA_INFO_EXT, - .offset = 0, + // Where the rewritten raygen reads the TLAS address: 0 when + // VulkanShader synthesized a fresh block, or the offset of + // the member it appended to the shader's existing block. + .offset = Device::workaroundTlasPushOffset, .data = { .address = &tlasAddr, .size = sizeof(tlasAddr) }, }; Device::vkCmdPushDataEXT(cmd, &pushInfo); diff --git a/interfaces/Crafter.Graphics-ShaderVulkan.cppm b/interfaces/Crafter.Graphics-ShaderVulkan.cppm index 7bff83f..49a6699 100644 --- a/interfaces/Crafter.Graphics-ShaderVulkan.cppm +++ b/interfaces/Crafter.Graphics-ShaderVulkan.cppm @@ -42,22 +42,36 @@ import :Types; // // glslang has no GLSL spelling for that conversion, so we rewrite the compiled // SPIR-V at module-load time: every `OpLoad %accelStruct ` becomes a -// load of the TLAS device address from a synthesized push-constant block -// followed by OpConvertUToAccelerationStructureKHR. RTPass pushes the active -// frame's TLAS address into that push constant. Shaders that never touch an -// acceleration structure (no OpTypeAccelerationStructureKHR) are left untouched. -namespace WorkaroundNvidiaAS { +// load of the TLAS device address from a push-constant block followed by +// OpConvertUToAccelerationStructureKHR. RTPass pushes the active frame's TLAS +// address into that push constant. Shaders that never touch an acceleration +// structure (no OpTypeAccelerationStructureKHR) are left untouched. +// +// SPIR-V allows at most one push-constant variable per entry point, so we never +// add a second one: if the shader already declares a push-constant block we +// append a ulong member (the TLAS address) to the *existing* block and read +// from there; only shaders with no push constant of their own get a freshly +// synthesized single-member block. Its byte offset is the offset of that +// member (published via Crafter::Device::workaroundTlasPushOffset) which RTPass feeds to +// vkCmdPushDataEXT so the address lands where the rewritten load reads it. +// +// Exported so tests/PushConstantRewrite can drive Patch() over real compiled +// SPIR-V and check the result with spirv-val; nothing in the engine calls it +// from outside this file. Goes away with the rest of the workaround. +export namespace WorkaroundNvidiaAS { // SPIR-V numeric opcodes / enums used below. enum : std::uint32_t { OpEntryPoint = 15, OpCapability = 17, - OpTypeInt = 21, OpTypeStruct = 30, OpTypePointer = 32, + OpTypeInt = 21, OpTypeFloat = 22, OpTypeVector = 23, OpTypeMatrix = 24, + OpTypeArray = 28, OpTypeStruct = 30, OpTypePointer = 32, OpConstant = 43, OpVariable = 59, OpLoad = 61, OpAccessChain = 65, OpDecorate = 71, OpMemberDecorate = 72, OpConvertUToAccelerationStructureKHR = 4447, OpTypeAccelerationStructureKHR = 5341, CapabilityInt64 = 11, StorageClassPushConstant = 9, - DecorationBlock = 2, DecorationOffset = 35, + DecorationBlock = 2, DecorationMatrixStride = 7, + DecorationArrayStride = 6, DecorationOffset = 35, }; inline bool IsAnnotation(std::uint32_t op) { @@ -69,6 +83,10 @@ namespace WorkaroundNvidiaAS { using Instr = std::vector; + inline std::uint32_t AlignUp(std::uint32_t v, std::uint32_t a) { + return (v + a - 1u) & ~(a - 1u); + } + inline void Patch(std::vector& words) { if (words.size() < 5) return; // not a SPIR-V module we understand. @@ -82,23 +100,61 @@ namespace WorkaroundNvidiaAS { i += len; } - // ── Scan for the AS type, reusable int/long types+constants, and the - // section boundaries we need to insert into. + // ── Scan for the AS type, reusable int/long types+constants, any + // existing push-constant block, the type/decoration/constant tables + // needed to size that block, and the section boundaries to insert into. std::uint32_t asTypeId = 0, ulongTypeId = 0, uintTypeId = 0, uintZeroId = 0; + std::uint32_t existingPcVarId = 0, existingPcStructId = 0, existingPtrUlongId = 0; std::size_t lastCapIdx = 0, lastAnnotIdx = 0, firstFuncIdx = instrs.size(); std::size_t entryIdx = instrs.size(); + std::map typeInstr; // type-result-id → defining instr + std::map constU32; // OpConstant id → 32-bit value + std::map uintConstByValue; // uint value → OpConstant id + std::map arrayStride; // array type id → ArrayStride + std::map memberOffset; // (struct<<32|idx) → Offset + std::map memberMatStride; // (struct<<32|idx) → MatrixStride + std::map ptrPointee; // pointer type id → pointee type id for (std::size_t k = 0; k < instrs.size(); ++k) { - std::uint32_t op = instrs[k][0] & 0xFFFFu; + const Instr& in = instrs[k]; + std::uint32_t op = in[0] & 0xFFFFu; switch (op) { - case OpTypeAccelerationStructureKHR: asTypeId = instrs[k][1]; break; + case OpTypeAccelerationStructureKHR: asTypeId = in[1]; typeInstr[in[1]] = ∈ break; case OpTypeInt: - if (instrs[k][2] == 64 && instrs[k][3] == 0) ulongTypeId = instrs[k][1]; - else if (instrs[k][2] == 32 && instrs[k][3] == 0) uintTypeId = instrs[k][1]; + if (in[2] == 64 && in[3] == 0) ulongTypeId = in[1]; + else if (in[2] == 32 && in[3] == 0) uintTypeId = in[1]; + typeInstr[in[1]] = ∈ + break; + case OpTypeFloat: case OpTypeVector: case OpTypeMatrix: + case OpTypeArray: case OpTypeStruct: + typeInstr[in[1]] = ∈ + break; + case OpTypePointer: + typeInstr[in[1]] = ∈ ptrPointee[in[1]] = in[3]; + if (in[2] == StorageClassPushConstant && in[3] == ulongTypeId) + existingPtrUlongId = in[1]; break; case OpConstant: - if (uintTypeId && instrs[k][1] == uintTypeId && instrs[k][3] == 0) - uintZeroId = instrs[k][2]; + if (in.size() >= 4) constU32[in[2]] = in[3]; + if (uintTypeId && in[1] == uintTypeId && in.size() >= 4) { + uintConstByValue.emplace(in[3], in[2]); + if (in[3] == 0) uintZeroId = in[2]; + } break; + case OpVariable: + if (in[3] == StorageClassPushConstant) { + existingPcVarId = in[2]; + existingPcStructId = ptrPointee.count(in[1]) ? ptrPointee[in[1]] : 0; + } + break; + case OpDecorate: + if (in.size() >= 4 && in[2] == DecorationArrayStride) arrayStride[in[1]] = in[3]; + break; + case OpMemberDecorate: { + std::uint64_t key = (static_cast(in[1]) << 32) | in[2]; + if (in.size() >= 5 && in[3] == DecorationOffset) memberOffset[key] = in[4]; + if (in.size() >= 5 && in[3] == DecorationMatrixStride) memberMatStride[key] = in[4]; + break; + } case OpCapability: lastCapIdx = k; break; case OpEntryPoint: if (entryIdx == instrs.size()) entryIdx = k; break; default: break; @@ -116,73 +172,153 @@ namespace WorkaroundNvidiaAS { return in; }; - // ── Synthesize the types/constants/push-constant we need, reusing any - // the module already defines (SPIR-V forbids duplicate type defs). - std::vector typeDefs; - if (uintTypeId == 0) { - uintTypeId = newId(); - typeDefs.push_back(mk({OpTypeInt, uintTypeId, 32, 0})); - } - if (uintZeroId == 0) { - uintZeroId = newId(); - typeDefs.push_back(mk({OpConstant, uintTypeId, uintZeroId, 0})); - } - if (ulongTypeId == 0) { - ulongTypeId = newId(); - typeDefs.push_back(mk({OpTypeInt, ulongTypeId, 64, 0})); - } - std::uint32_t pcStructId = newId(); - std::uint32_t ptrPushStructId = newId(); - std::uint32_t ptrPushUlongId = newId(); - std::uint32_t pcVarId = newId(); - typeDefs.push_back(mk({OpTypeStruct, pcStructId, ulongTypeId})); - typeDefs.push_back(mk({OpTypePointer, ptrPushStructId, StorageClassPushConstant, pcStructId})); - typeDefs.push_back(mk({OpTypePointer, ptrPushUlongId, StorageClassPushConstant, ulongTypeId})); - typeDefs.push_back(mk({OpVariable, ptrPushStructId, pcVarId, StorageClassPushConstant})); - - std::vector decorations = { - mk({OpMemberDecorate, pcStructId, 0, DecorationOffset, 0}), - mk({OpDecorate, pcStructId, DecorationBlock}), + // Byte footprint of a type, honouring the explicit Array/Matrix strides + // glslang emits so the result is correct under both scalar and std140 + // block layout. Used only to find where an existing push block ends. + std::function footprint = + [&](std::uint32_t tid) -> std::uint32_t { + auto it = typeInstr.find(tid); + if (it == typeInstr.end()) return 0; + const Instr& t = *it->second; + switch (t[0] & 0xFFFFu) { + case OpTypeInt: case OpTypeFloat: return t[2] / 8u; + case OpTypeVector: return t[3] * footprint(t[2]); + case OpTypeMatrix: return t[3] * footprint(t[2]); // cols × column-vec + case OpTypeArray: { + std::uint32_t len = constU32.count(t[3]) ? constU32[t[3]] : 0; + std::uint32_t stride = arrayStride.count(tid) ? arrayStride[tid] + : footprint(t[2]); + return len * stride; + } + case OpTypeStruct: { + std::uint32_t end = 0; + for (std::size_t m = 2; m < t.size(); ++m) { + std::uint32_t idx = static_cast(m - 2); + std::uint64_t key = (static_cast(t[1]) << 32) | idx; + std::uint32_t off = memberOffset.count(key) ? memberOffset[key] : 0; + std::uint32_t sz; + auto mt = typeInstr.find(t[m]); + if (mt != typeInstr.end() && (mt->second->at(0) & 0xFFFFu) == OpTypeMatrix + && memberMatStride.count(key)) + sz = memberMatStride[key] * (*mt->second)[3]; + else + sz = footprint(t[m]); + end = std::max(end, off + sz); + } + return end; + } + case OpTypePointer: return 8; + default: return 0; + } }; - // ── Rewrite each `OpLoad %asType ` into address-load + convert. + bool merge = existingPcVarId != 0 && existingPcStructId != 0 + && typeInstr.count(existingPcStructId) + && (typeInstr[existingPcStructId]->at(0) & 0xFFFFu) == OpTypeStruct; + + // ── Synthesize/ensure the int/long types and constants we need, reusing + // any the module already defines (SPIR-V forbids duplicate type defs). + std::vector typeDefs; + if (uintTypeId == 0) { uintTypeId = newId(); typeDefs.push_back(mk({OpTypeInt, uintTypeId, 32, 0})); } + if (ulongTypeId == 0) { ulongTypeId = newId(); typeDefs.push_back(mk({OpTypeInt, ulongTypeId, 64, 0})); } + + std::uint32_t pcVarId, ptrPushUlongId, memberIdxConstId, memberIdx; + std::vector decorations; + + if (merge) { + // Append a ulong member to the user's existing block; read from it. + pcVarId = existingPcVarId; + const Instr* structInstr = typeInstr[existingPcStructId]; + memberIdx = static_cast(structInstr->size() - 2); + Crafter::Device::workaroundTlasPushOffset = AlignUp(footprint(existingPcStructId), 8); + + ptrPushUlongId = existingPtrUlongId; + if (ptrPushUlongId == 0) { + ptrPushUlongId = newId(); + typeDefs.push_back(mk({OpTypePointer, ptrPushUlongId, StorageClassPushConstant, ulongTypeId})); + } + // Member index constant for the access chain — reuse an existing + // uint constant of the right value, else mint one (must be an + // integer constant, so only uint-typed ones qualify for reuse). + auto found = uintConstByValue.find(memberIdx); + if (found != uintConstByValue.end()) { + memberIdxConstId = found->second; + } else { + memberIdxConstId = newId(); + typeDefs.push_back(mk({OpConstant, uintTypeId, memberIdxConstId, memberIdx})); + } + decorations.push_back(mk({OpMemberDecorate, existingPcStructId, memberIdx, DecorationOffset, Crafter::Device::workaroundTlasPushOffset})); + } else { + // No user push constant — synthesize a fresh single-member block. + if (uintZeroId == 0) { uintZeroId = newId(); typeDefs.push_back(mk({OpConstant, uintTypeId, uintZeroId, 0})); } + std::uint32_t pcStructId = newId(); + std::uint32_t ptrPushStructId = newId(); + ptrPushUlongId = newId(); + pcVarId = newId(); + typeDefs.push_back(mk({OpTypeStruct, pcStructId, ulongTypeId})); + typeDefs.push_back(mk({OpTypePointer, ptrPushStructId, StorageClassPushConstant, pcStructId})); + typeDefs.push_back(mk({OpTypePointer, ptrPushUlongId, StorageClassPushConstant, ulongTypeId})); + typeDefs.push_back(mk({OpVariable, ptrPushStructId, pcVarId, StorageClassPushConstant})); + decorations.push_back(mk({OpMemberDecorate, pcStructId, 0, DecorationOffset, 0})); + decorations.push_back(mk({OpDecorate, pcStructId, DecorationBlock})); + memberIdxConstId = uintZeroId; + Crafter::Device::workaroundTlasPushOffset = 0; + } + + // ── Rewrite each `OpLoad %asType ` into address-load + convert, and + // (when merging) append the ulong member to the existing struct type. std::vector rebuilt; rebuilt.reserve(instrs.size() + 8); - for (const Instr& in : instrs) { + for (Instr in : instrs) { std::uint32_t op = in[0] & 0xFFFFu; if (op == OpLoad && in[1] == asTypeId) { std::uint32_t resultId = in[2]; std::uint32_t chainId = newId(); std::uint32_t addrId = newId(); - rebuilt.push_back(mk({OpAccessChain, ptrPushUlongId, chainId, pcVarId, uintZeroId})); + rebuilt.push_back(mk({OpAccessChain, ptrPushUlongId, chainId, pcVarId, memberIdxConstId})); rebuilt.push_back(mk({OpLoad, ulongTypeId, addrId, chainId})); rebuilt.push_back(mk({OpConvertUToAccelerationStructureKHR, asTypeId, resultId, addrId})); } else { - rebuilt.push_back(in); + if (merge && op == OpTypeStruct && in[1] == existingPcStructId) { + in.push_back(ulongTypeId); + in[0] = static_cast(in.size() << 16) | OpTypeStruct; + } + rebuilt.push_back(std::move(in)); } } instrs.swap(rebuilt); // Recompute structural anchors (the rewrite above shifted indices). lastCapIdx = 0; lastAnnotIdx = 0; firstFuncIdx = instrs.size(); entryIdx = instrs.size(); + std::size_t structIdx = instrs.size(); for (std::size_t k = 0; k < instrs.size(); ++k) { std::uint32_t op = instrs[k][0] & 0xFFFFu; if (op == OpCapability) lastCapIdx = k; if (op == OpEntryPoint && entryIdx == instrs.size()) entryIdx = k; if (IsAnnotation(op)) lastAnnotIdx = k; if (op == 54 && firstFuncIdx == instrs.size()) firstFuncIdx = k; + if (merge && op == OpTypeStruct && instrs[k][1] == existingPcStructId) structIdx = k; } - // Append the push-constant variable to the entry point's interface - // list (required for SPIR-V ≥ 1.4 — both raygen modules are 1.4). - if (entryIdx != instrs.size() && words[1] >= 0x00010400u) { + // The newly-defined types (notably ulong) must precede every use. When + // merging, the user's struct — now carrying the appended ulong member — + // already sits in the type section, so the defs go in just before it; + // for a fresh block the whole bundle can go at the end of the type + // section (right before the first function). + std::size_t typeDefsIdx = (merge && structIdx != instrs.size()) ? structIdx : firstFuncIdx; + + // A freshly synthesized push-constant variable must join the entry + // point's interface list (required for SPIR-V ≥ 1.4 — raygen is 1.4). + // A merged-into variable is already used, so it is already listed. + if (!merge && entryIdx != instrs.size() && words[1] >= 0x00010400u) { instrs[entryIdx].push_back(pcVarId); instrs[entryIdx][0] = static_cast(instrs[entryIdx].size() << 16) | OpEntryPoint; } - // Insert highest-index-first so earlier anchors stay valid. - instrs.insert(instrs.begin() + firstFuncIdx, typeDefs.begin(), typeDefs.end()); + // Insert highest-index-first so earlier anchors stay valid (typeDefsIdx + // ≥ lastAnnotIdx+1 ≥ lastCapIdx+1 in both the merge and synthesize cases). + instrs.insert(instrs.begin() + typeDefsIdx, typeDefs.begin(), typeDefs.end()); instrs.insert(instrs.begin() + lastAnnotIdx + 1, decorations.begin(), decorations.end()); instrs.insert(instrs.begin() + lastCapIdx + 1, mk({OpCapability, CapabilityInt64}));