Zero-initialize struct-typed function-local variables (fixes Area Lights, OpenPBR Analytic Lights)#1720
Closed
bkaradzic-microsoft wants to merge 1 commit into
Conversation
Babylon.js emits shaders such as `computeAreaLighting` (inlined into the
fragment shader by every Area Lights / Standard Material scene) that
declare a local struct without an initializer and immediately do
read-modify-write on its fields, e.g.
lightingInfo result;
result.specular += value;
result.diffuse += value;
return result;
This is undefined behavior under the GLSL spec (locals are not
zero-initialized), but every WebGL driver in practice tolerates it. The
HLSL that SPIRV-Cross emits faithfully reproduces the pattern, and
D3DCompile correctly rejects it with `error X4000: variable used without
having been completely initialized`. The result is that Area Lights -
Standard Material (and any other test that exercises this code path)
fails to compile its fragment shader on D3D11 / D3D12.
Add a new AST traverser, `ZeroInitializeStructLocals`, that walks every
function body and prepends an `EOpAssign(symbol, EOpConstructStruct(0,
0, ...))` for each unique struct-typed `EvqTemporary` symbol it
references. The SPIR-V emitter then sees a normal store; SPIRV-Cross
emits HLSL with the struct local definitely initialized; D3DCompile
accepts it. Any subsequent real assignment in the body is dead-code
eliminated by DXC's optimizer, so functional behavior is unchanged.
Wired into the DXBC, DXIL, Metal and Vulkan compile pipelines (same as
the existing sampler-handling and dFdy traversers). Arrays of structs
are intentionally skipped — they are uncommon in BJS shaders and would
require constructing a synthetic array constructor that does not match
any GLSL syntax.
Verified end-to-end on Win32 D3D11 RelWithDebInfo / Chakra:
idx 191 Area Lights - Standard Material : PASS (was X4000 in
D3DCompile)
idx 194 Area Lights - PBR : PASS (regression check
for BabylonJS#1718)
idx 0/39/47/165 : PASS (baseline)
134/135 wide regression sweep : PASS (only failure is
the pre-existing
idx 22 Outline JS
error, unrelated)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Closed and superseded by #1721 (branch was renamed for housekeeping). |
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
Babylon.js v6+ shaders for Area Lights (
computeAreaLighting) and a number of other PBR/OpenPBR helpers declare a struct local without an initializer, then immediately do read-modify-write on its fields:This is undefined behavior under the GLSL spec (locals are not zero-initialized), but every WebGL driver in practice tolerates it. SPIRV-Cross faithfully reproduces the pattern in its HLSL output, and
D3DCompilecorrectly rejects it witherror X4000: variable used without having been completely initialized. Result:Area Lights - Standard Materialand any other test that exercises this code path fails to compile its fragment shader on D3D11 / D3D12 today.Fix
Add a new AST traverser,
ZeroInitializeStructLocals, that walks every function body and prependsEOpAssign(symbol, EOpConstructStruct(0, 0, ...))for each unique struct-typedEvqTemporarysymbol it references. The SPIR-V emitter then sees a normal store; SPIRV-Cross emits HLSL with the struct local definitely initialized;D3DCompileaccepts it. Any subsequent real assignment in the body is dead-code eliminated by DXC's optimizer, so functional behavior is unchanged.Wired into the DXBC, DXIL, Metal, and Vulkan compile pipelines (same as the existing sampler-handling and dFdy traversers). Arrays of structs are intentionally skipped — they are uncommon in BJS shaders and would require constructing a synthetic array constructor.
Verification
End-to-end on
build/win32-default(D3D11, Chakra, RelWithDebInfo) andbuild/win32-gl(OpenGL/ANGLE, V8, Release):Wide regression sweep:
OutlinewithError: Unsupported alpha mode: -1— pre-existing, unrelated to shader pipeline.No new regressions on either backend.
Why a BN traverser (not a SPIRV-Cross fix)
Both the SPIRV-Cross fork-extension route and the BN traverser route would solve this. We chose the traverser because:
Out of scope
Textured - Area Lights - *) still fail withUnsupported texture format or type: format 5, type 3553. This is aNativeEngine::PrepareImagewhitelist gap and orthogonal to this PR.TypeError: 'FLOAT' undefinedbefore any shader compile. Will be filed as a separate issue.