SPIRV: hoist constant indexable rvalue temps to Private storage#4237
Open
tycho wants to merge 2 commits into
Open
SPIRV: hoist constant indexable rvalue temps to Private storage#4237tycho wants to merge 2 commits into
tycho wants to merge 2 commits into
Conversation
When a const-qualified array (or any rvalue constant composite) is indexed by a runtime expression, glslang materialized the constant into a fresh Function-storage OpVariable at every access site, with an OpStore of the constant immediately preceding the OpAccessChain. This caused a per-invocation memcpy of the entire constant and was catastrophic on real hardware for non-trivial array sizes (~480x slower at 720p on NVIDIA for one shadertoy with two 256-element vec3 constants accessed at 4 sites each). This change emits the temp as a single module-scope Private-storage OpVariable with the constant as its Initializer, memoized per distinct constant. The pattern is well-formed under SPIR-V 1.0 and is the canonical form drivers recognize as an immutable lookup table. ANGLE landed essentially the same fix in its own SPIR-V translator; this patch mirrors that approach. For SPIR-V 1.4+, where statically-used global variables must appear in the entry point interface list, the hoisted vars are merged into iOSet before OpEntryPoint is finalized. Spec constants are intentionally excluded from the hoist: their value can change at pipeline creation, and hoisting them would change their identity from per-invocation to module-shared. Adds Test/spv.privateHoist.frag covering the memoization invariant (two distinct constants with four access sites each must produce exactly two module-scope Private variables and zero OpStores of either constant). Existing goldens regenerated for tests that hit the affected code path; all diffs are the mechanical storage-class change and, for SPIR-V 1.4+ tests, the addition of the new Id to the OpEntryPoint interface list.
The previous 1.4+ branch in Builder::accessChainLoad fired when isValidInitializer(accessChain.base) returned true, defined as isConstant(id) || isGlobalVariable(id). After the preceding hoist: - The constant disjunct is handled by the hoist above the branch. - The global-variable disjunct is structurally unreachable. accessChain.base reaches this code via setAccessChainRValue, whose 18 callers in GlslangToSpv.cpp (the unified GLSL+HLSL emitter) all pass value-typed Ids. The dispatch in visitSymbol explicitly forks on isPointerType: pointer-typed Ids (which all global variables are) take the lvalue path. No caller passes a global-variable Id to the rvalue path. The branch is dead from this site. The isValidInitializer helper itself remains valid in the variable-creation contexts it was written for; only this one usage was over-broad. Test suites are unchanged: 1885/1885 gtests and the shell testsuite both pass without modification.
dnovillo
requested changes
May 26, 2026
dnovillo
left a comment
Collaborator
There was a problem hiding this comment.
Looks largely OK. Thanks for the detailed description!
| bool hoistableConstant = | ||
| baseOp == Op::OpConstantComposite || | ||
| baseOp == Op::OpConstantCompositeReplicateEXT || | ||
| baseOp == Op::OpConstantNull; |
Collaborator
There was a problem hiding this comment.
nit: could you add an explanation on why hoistableConstant is excluding OpSpecConstant*. The PR description states it, but for future changes, it will be useful to have a reminder here on the side-effects of changing the constant at pipeline creation.
Collaborator
There was a problem hiding this comment.
Also, this may read easier if it was factored into a separate helper. Makes the helper testable from Builder too.
| @@ -0,0 +1,41 @@ | |||
| #version 450 | |||
Collaborator
There was a problem hiding this comment.
Could you add a negative test for spec constants here?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a
const-qualified array (or any rvalue constant composite) is indexed by a runtime expression, glslang materializes the constant into a fresh Function-storageOpVariableat every access site, with anOpStoreof the constant immediately preceding theOpAccessChain. This causes a per-invocation memcpy of the entire constant and is catastrophic on real hardware for non-trivial array sizes.This change emits the temp as a single module-scope Private-storage
OpVariablewith the constant as itsInitializer, memoized per distinct constant. The pattern is well-formed under SPIR-V 1.0 and is a long-recognized form for an immutable lookup table.Bug
Minimal repro (fragment shader,
--target-env opengl, SPIR-V 1.0):Before this patch:
For each access site of each constant, glslang emits a fresh Function variable and a fresh
OpStoreof the entire constant. With multiple access sites of large constants, the per-invocationOpStoretraffic scales accordingly.The bug was originally observed on a real shader, the Jupiter and Io shadertoy by mtnierhoff (https://www.shadertoy.com/view/XXjSRc), running about 480x slower than necessary at 720p on NVIDIA. Buffer A uses two 256-element vec3 constants (about 6 KiB of constant data total). Each is accessed at 4 sites in two helper functions; before this patch, each fragment invocation emitted one Function-storage indexable temp and one
OpStore-of-the-full-constant per access site, for a total of about 24 KiB ofOpStoretraffic per invocation before any sampling work began.The same shader runs at full speed under D3D11; the DXIL/HLSL toolchain does not emit this pattern. The bug was first reproduced under ANGLE's Vulkan backend, which uses ANGLE's own GLSL-to-SPIR-V translator rather than glslang. ANGLE's translator emitted the same
OpStore-of-constant pattern that glslang's pre-1.4 fallback does, and the speedup numbers above come from fixing that pattern in ANGLE. The author of ANGLE's translator (@ShabbyX) suggested checking whether glslang has the analogous bug, which is what produced this patch. The before/after SPIR-V from glslang on this shader and on the affected Buffer A is byte-equivalent in shape to ANGLE's pre/post-fix output.The behavior of NVIDIA, or any other driver, on the existing SPIR-V 1.4
OpVariable Function ... Initializer + NonWritableform is not measured here. The argument for replacing that form is that the new encoding gives the optimizer strictly more information without relying on any driver-side heuristic, and that it also fixes the original bug for pre-1.4 targets which the existing optimization could not reach.Why not fix this in spirv-opt
Verified against
spirv-opt -O:--scalar-replacementbails because indices are runtime-computed.--eliminate-local-single-storecannot foldOpLoad(OpAccessChain(var, runtime_idx))toOpCompositeExtractbecause the latter requires literal indices.--copy-propagate-arrayshandles whole-array copies, not this access pattern.No combination of stock SPIRV-Tools passes recovers the lookup table. The fix needs to live in the front-end.
Prior art
ANGLE landed essentially this fix in its own SPIR-V translator, in two commits earlier today:
This patch mirrors that approach in glslang.
What the patch does
In
Builder::accessChainLoad(SPIRV/SpvBuilder.cpp), when the rvalue base of an access chain with a runtime index is anOpConstantComposite,OpConstantCompositeReplicateEXT, orOpConstantNull, emit (or look up a memoized) module-scopeOpVariable %ptr Private %const. Use that as theOpAccessChainbase. NoOpStoreneeded.After this patch, the same minimal shader emits:
The constant is bound to a module-scope Private variable via the
Initializeroperand. NoOpStoreof the constant; no per-invocation copy.Note that the constant value itself was already module-scope before this patch. SPIR-V has no concept of a function-scoped constant; every
OpConstant*instruction lives at module scope, regardless of where the GLSL source declared the array. What was function-scoped before this patch was the wrapperOpVariableglslang emitted to give the constant an addressable home forOpAccessChain. This patch moves that wrapper variable to module scope as well.Memoization across access sites and across functions
N access sites of the same constant share one variable, including across different functions in the shader. New test
Test/spv.privateHoist.fragcovers this:The relevant lines of the emitted SPIR-V (with
--target-env opengl):Eight
OpAccessChainoperations across two functions, two module-scope Private variables, zeroOpStoreinstructions targeting either variable.helper()andmain()share the same%indexablefortableA. Without memoization this would have been eight distinct Function variables and eightOpStoreoperations. On the affected shadertoy, memoization collapses the 8 access sites of the two 256-element vec3 arrays from 8 indexable temps to 2.Entry point interface
For SPIR-V 1.4 and later, statically-used global variables must appear in the entry point interface list. The hoisted vars are merged into
iOSetinTGlslangToSpvTraverser(SPIRV/GlslangToSpv.cpp) beforeOpEntryPointis finalized. For pre-1.4 targets only Input/Output variables need to appear, so the interface list is unchanged on those targets.Spec constants
OpSpecConstant*is intentionally excluded. Spec constants can change at pipeline creation time, and hoisting them changes their identity from per-invocation to module-shared.Comparison with the existing SPIR-V 1.4 Function-with-initializer encoding
Pre-patch, glslang already had a partial optimization at the same call site, gated on SPIR-V 1.4 or later: emit
OpVariable Function %constwithNonWritable, noOpStore. The new encoding supersedes that one for the only case it actually fired on. Differences worth calling out:Storage class lifetime. Function-storage variables are local to a function call; their
Initializeroperand says "set this on entry to the function." Private-storage variables are module-scope with per-invocation lifetime; theirInitializeris applied once at the start of the invocation and the variable persists across the function calls that invocation makes. For a helper function called N times, the old encoding can require up to N initializations; the new one can require up to 1.Memoization. Function variables must be declared in the entry block of their owning function; two functions cannot share one Function variable. The old encoding therefore could not deduplicate the same constant being indexed from multiple functions. Private variables are module-scope and can be shared by every function in the shader, which is the basis for the memoization map this patch adds.
SPIR-V version reach. The old encoding required SPIR-V 1.4 because
NonWritableon a Function variable is only legal there. Below 1.4, glslang fell back to the explicitOpStoreform, which is the form the original bug arises from. The new encoding works on SPIR-V 1.0 and later, and so fixes the bug on--target-env opengland--target-env vulkan1.0/vulkan1.1(default SPIR-V 1.0 / 1.3) as well.Decorations. The old encoding decorated the Function variable
NonWritableto telegraph "lookup table." The new encoding does not need a decoration: the constantInitializerplus the absence of anyOpStoreto the variable is itself a strong static signal of immutability, andNonWritableis not a legal decoration on a Private variable in any current SPIR-V version anyway.The new encoding is at least as good in every dimension the spec lets us reason about, and strictly better on the ones a driver cannot avoid: memoization across access sites is a SPIR-V structural property, not a driver heuristic.
Removing the now-unreachable 1.4+ branch
The previous 1.4+ branch fired when
isValidInitializer(accessChain.base)returned true, defined asisConstant(id) || isGlobalVariable(id). After the patch:accessChain.basereaches this code viasetAccessChainRValue, which has 18 callers inGlslangToSpv.cpp(the unified GLSL+HLSL emitter; HLSL has no separate emitter). Every caller passes a value-typed Id (load result, arithmetic result, constructor result, constant). The dispatch invisitSymbolat GlslangToSpv.cpp:2283 explicitly forks onisPointerType: pointer-typed Ids, which all global variables are, take the lvalue path. No caller passes a global-variable Id to the rvalue path.The branch is dead from this site. The
isValidInitializerpredicate itself remains valid in the variable-creation contexts it was originally written for; only this one usage was over-broad.This cleanup is in a separate commit. If maintainers prefer to land just the hoist for now, the second commit reverts cleanly.
Validation
glslang-gtests: 1907 / 1907 pass. 13 existing goldens regenerated for tests that hit the affected code path; all diffs are the mechanical storage-class change (Variable Function <ConstId>becomingVariable Private <ConstId>at module scope) and, for SPIR-V 1.4+ tests, the addition of the new Id to theOpEntryPointinterface list. No semantic differences.Test/runtests(shell suite): "Tests Succeeded."spv.privateHoist.fragcovers the cross-function memoization invariant.Tested targets
--target-env opengl--target-env vulkan1.0--target-env vulkan1.1--target-env vulkan1.1 --target-env spirv1.4--target-env vulkan1.2--target-env vulkan1.3Note: the
vulkan1.1 + spirv1.4row's module is validated undervulkan1.2semantics, since SPIR-V 1.4 requires Vulkan 1.2 orVK_KHR_spirv_1_4to be valid in the host environment.