fix: implement missing GLSL.std.450 opcodes (#334) - #350
Conversation
…reter Stop silent zero returns for unimplemented ExtInst ops (#334): align Khronos opcode numbers, warn on unknowns, and implement matrix/pack/split/bit/geom ops with ≥86% package coverage.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
kolkov
left a comment
There was a problem hiding this comment.
Good work on filling the GLSL.std.450 gaps — the opcode renumbering from 76/77 to 33/34 is a critical fix (verified against GLSL.std.450.h), and the coverage of pack/unpack/matrix/split/bit-find is thorough.
Deep review found 2 critical bugs (empirically validated):
BUG 1 (Critical, VALIDATED): inverse3 and inverse4 missing cofactor transpose
glsl_matrix.go — the adjugate matrix is the transpose of the cofactor matrix. The output loop reads cof[0][c], cof[1][c], cof[2][c] (column c of cofactor), but should read cof[c][0], cof[c][1], cof[c][2] (row c of cofactor = column c of adjugate).
Empirical proof: for the non-symmetric matrix [[1,2,3],[0,1,4],[5,6,0]] (column-major), A * inv(A) produces garbage with the current code:
27.0000 -22.0000 6.0000
38.0000 -31.0000 8.0000
-12.0000 10.0000 -1.0000
After transposing the indices, it produces identity. Same confirmed for 4x4.
Tests pass because they only test diagonal/identity matrices where A^T = A — the transpose bug is invisible.
inverse2 is correct (hardcoded adjugate formula, no loop).
Fix for inverse3:
for c := 0; c < 3; c++ {
out[c] = ValVec3(cof[c][0]*inv, cof[c][1]*inv, cof[c][2]*inv)
}Fix for inverse4:
for c := 0; c < 4; c++ {
out[c] = ValVec4(cof[c][0]*inv, cof[c][1]*inv, cof[c][2]*inv, cof[c][3]*inv)
}Add a non-symmetric test case to prevent regression.
BUG 2 (Critical, VALIDATED): float32ToFloat16bits denorm path zeroes all denorms
glsl_pack.go, float32ToFloat16bits, case exp < 113 branch:
mant |= 0x800000 // 24 bits
shift := uint32(126 - exp) // shift ∈ [14,23]
mant = (mant + (1 << (shift - 1))) >> shift // now [1..10] bits
return sign | uint16(mant>>13) // BUG: >>13 kills resultAfter >> shift, mantissa is already in 10-bit range. The extra >> 13 (correct for the normal path's 23→10 bit reduction) pushes all denorms to zero.
Empirical proof: exhaustive test of 6 denorm inputs — all produce 0x0000 with original, all produce correct bits with uint16(mant):
input ORIG FIXED expected
3.051758e-05 0x0000 0x0200 0x0200
3.814697e-06 0x0000 0x0040 0x0040
5.960464e-08 0x0000 0x0001 0x0001
4.577637e-05 0x0000 0x0300 0x0300
Fix:
return sign | uint16(mant)NOT A BUG (reviewed and dismissed):
float16bitsToFloat32 denorm uint16 wrap — initially looked like a bug (exp-- wraps uint16 from 0 to 65535), but exhaustive test of all 1023 f16 denorms shows zero mismatches between uint16 and int versions. The uint32 overflow in fexp << 23 accidentally produces correct IEEE-754 bits due to modular arithmetic. Not a bug, though using int would be cleaner.
Minor (not blocking):
findILsb/findSMsb/findUMsbonly handle scalar operands. SPIR-V GLSL.std.450 spec says these accept "scalar or vector of integer type" and return component-wise results. Fine for current usage, but worth a TODO.
Summary: Fix BUG 1 (inverse transpose) + BUG 2 (pack denorm >>13) + add non-symmetric matrix test. Everything else is verified correct — opcode numbers match Khronos header, NaN-aware min/max matches spec, snorm clamping correct, refract formula correct.
Address #350 review: transpose cofactors in inverse3/4, drop the extra >>13 on half denorms, use int for unpack exp, and trim stale local/RU links from VALIDATION-TESTING.md.
|
@kolkov Fixed comments. Round 2 Ready |
kolkov
left a comment
There was a problem hiding this comment.
All 3 fixes verified locally: inverse3/4 transpose, f16 pack denorm, f16 unpack int type. Non-symmetric matrix tests + denorm roundtrip tests added. 22/22 packages pass, 0 lint issues.
Summary
Determinant/MatrixInversewere 76/77; correct values are 33/34) and implement the remaining GLSL.std.450 ExtInst ops in the software SPIR-V interpreter.hal.Logger().Warnon unknown opcodes to stop silent zero corruption on CI/servers/software backend.Closes #334
Test plan
go test ./hal/software/shader— coverage 86.2% (≥86%)go test ./...golangci-lint run --timeout=5m