diff --git a/CMakeLists.txt b/CMakeLists.txt index 00ea130e..d64f3f5e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -301,6 +301,8 @@ set(SOURCE_FILES MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripNoPerspectivePass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EmulateNoPerspectivePass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeFragmentOutputIndexPass.cpp + MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.cpp + MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.cpp MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.cpp diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index a8fe42d3..2dbc8753 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -2134,19 +2134,31 @@ namespace MobileGL::MG_Backend::DirectGLES { if (tailSpanDirty) { // Scissor box. Resolved and shadowed like the viewport above, and // for the same reason: what has to reach the driver is NOT simply the parameter - // field. (0,0,0,0) is where RenderStateParameters::ScissorBox starts and the only - // thing that ever writes it is glScissor, so that value means "the application has - // never called glScissor" - it is not a GL scissor box. GL's initial box is the - // whole window, which the frontend has no way to spell before a surface exists. - // The pre-resync code got away with pushing the field verbatim only by accident: - // the shadow held the same default, the field never compared unequal, and the ES - // context kept its own correct default. Under the forced full push that accident - // is gone, glScissor(0,0,0,0) shrinks the scissor to an EMPTY rectangle, and - // everything drawn with GL_SCISSOR_TEST enabled before the app's first glScissor - // is clipped away - Minecraft 26.2 keeps only its unscissored sky and hand and - // loses the terrain and the whole GUI. + // field. RenderStateParameters::ScissorBoxes starts all-zero, which means "the + // application has never called glScissor" - it is not a GL scissor box. GL's + // initial box is the whole window, which the frontend has no way to spell before a + // surface exists. The pre-resync code got away with pushing the field verbatim only + // by accident: the shadow held the same default, the field never compared unequal, + // and the ES context kept its own correct default. Under the forced full push that + // accident is gone, glScissor(0,0,0,0) shrinks the scissor to an EMPTY rectangle, + // and everything drawn with GL_SCISSOR_TEST enabled before the app's first + // glScissor is clipped away - Minecraft 26.2 keeps only its unscissored sky and + // hand and loses the terrain and the whole GUI. + // + // The condition is the WRITTEN FLAG, not the extent. An empty rectangle is a + // perfectly legal thing to ask for - glScissor(0,0,0,0) means "the scissor test + // rejects every fragment" - so testing `width <= 0 || height <= 0` substituted the + // whole surface for a deliberately empty box and inverted the request into "accept + // every fragment", no matter how many times the application had already called + // glScissor. KHR-GL43.viewport_array.scissor_zero_dimension is exactly that: all 16 + // boxes zero-sized with the test enabled, requiring the draw to be clipped away + // entirely. Reading the flag preserves the Minecraft protection bit-for-bit - before + // the first glScissor the bit is clear and the surface size is still substituted - + // while an explicit empty box now reaches the driver verbatim. Negative extents + // cannot arrive here at all: all three entry points reject them with + // GL_INVALID_VALUE before storing (GL_RenderState.cpp's ValidateNonNegativeExtent). IntVec4 backendScissorBox = parameters.ScissorBoxes[0]; - if (backendScissorBox.z() <= 0 || backendScissorBox.w() <= 0) { + if ((parameters.ScissorBoxWrittenMask & 1u) == 0) { Int surfaceWidth = 0; Int surfaceHeight = 0; if (QueryCurrentSurfaceSize(surfaceWidth, surfaceHeight)) { @@ -5894,31 +5906,19 @@ namespace MobileGL::MG_Backend::DirectGLES { if (!out.texture) return false; const TextureTarget stateTarget = MG_Util::ConvertGLEnumToTextureTarget(appTarget); out.target = TextureImpl::ConvertTextureTargetToBackendGLEnum(stateTarget); - if (stateTarget == TextureTarget::Texture1DArray) { - out.x = x; - out.y = 0; - out.z = y; - return true; - } + // No axis remap for GL_TEXTURE_1D_ARRAY. The frontend STORES a 1D array with its layers + // on y (GetBackendUploadSize moves them across to the ES 2D array's z), but this entry + // point does not ADDRESS it that way: GL 4.6 core 18.3.2 treats every array texture as a + // stack of slices on z and gives a 1D array a height of 1 - exactly the shape the ES 2D + // array has - so GL's (x, 0, layer) and the ES image's (x, 0, layer) already agree. + // Remapping y into z here fetched the wrong slice for every call that spelled the layer + // the way GL defines it. out.x = x; out.y = y; out.z = z; return true; } - // The region extent swaps the same two axes for a 1D array, and does so for whichever side - // of the copy is one - GL forbids a copy whose two endpoints disagree about how many layers - // move, so at most one of the two can be a 1D array only in the degenerate single-layer - // case, where the swap is the identity anyway. - static void ApplyGLESCopyImageExtent(GLenum appSrcTarget, GLenum appDstTarget, GLsizei& height, GLsizei& depth) { - const TextureTarget srcStateTarget = MG_Util::ConvertGLEnumToTextureTarget(appSrcTarget); - const TextureTarget dstStateTarget = MG_Util::ConvertGLEnumToTextureTarget(appDstTarget); - if (srcStateTarget != TextureTarget::Texture1DArray && dstStateTarget != TextureTarget::Texture1DArray) { - return; - } - std::swap(height, depth); - } - static TextureInternalFormat GetCopyImageEndpointFormat(const CopyImageEndpoint& endpoint) { if (endpoint.IsRenderbuffer()) return endpoint.Renderbuffer->GetInternalFormat(); return endpoint.Texture ? endpoint.Texture->GetFormat() : TextureInternalFormat::Unknown; @@ -6029,9 +6029,10 @@ namespace MobileGL::MG_Backend::DirectGLES { return; } - GLsizei copyHeight = srcHeight; - GLsizei copyDepth = srcDepth; - ApplyGLESCopyImageExtent(srcTarget, dstTarget, copyHeight, copyDepth); + // Verbatim: GL already spells a 1D array's extent the way the ES 2D array it maps onto + // wants it (height 1, layers on depth) - see MakeGLESCopyImageEndpoint. + const GLsizei copyHeight = srcHeight; + const GLsizei copyDepth = srcDepth; const TextureInternalFormat srcFormat = GetCopyImageEndpointFormat(srcEndpoint); const TextureInternalFormat dstFormat = GetCopyImageEndpointFormat(dstEndpoint); diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 87c62590..0da1f4f2 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -396,6 +396,13 @@ namespace MobileGL::MG_Backend::DirectGLES { // entry would otherwise false-skip the rebind). void ScrubBufferBindingShadowsForId(Uint id); + // Defined next to the same shadow, and the counterpart to the scrub above for a + // buffer whose STORE was re-specified rather than deleted: the binding survives - + // nothing unbound the id - but the extent the driver resolved for it at bind time + // does not. Marks those points unknown so the next sync issues a real + // glBindBufferBase/Range instead of skipping it. + void InvalidateIndexedBufferBindingShadowsForId(Uint id); + // Resources whose owning BufferObject died; ids deleted at the next // sync point with a current ES context. Vector> g_deferredBufferReleases; @@ -568,6 +575,10 @@ namespace MobileGL::MG_Backend::DirectGLES { ZoneScopedC(TRACY_ZONECOLOR_BACKEND); #endif const SizeT size = bufferObject.GetSize(); + // Read BEFORE the fields below are overwritten: whether this respecify changes + // the store's EXTENT is what decides if the indexed-binding shadow still + // describes the driver. + const Bool extentChanged = !resource.storageInitialized || resource.storageSize != size; const GLenum usage = MG_Util::ConvertBufferUsageToGLEnum(bufferObject.GetUsage()); BindBufferId(TempBufferTarget, resource.id); // An orphaning respecify (glBufferData with NULL, content never @@ -583,6 +594,17 @@ namespace MobileGL::MG_Backend::DirectGLES { resource.pendingRespecify = false; resource.pendingRanges.clear(); resource.syncedChangeSerial = bufferObject.GetChangeSerial(); + // A GROWN store keeps its indexed bindings, and BindBufferBaseCached skips a + // rebind whenever the shadow already records this id at that index - so on a + // driver that resolves a whole-buffer indexed binding's extent at BIND time + // (Adreno does; Mali does not) the shader keeps seeing the old, smaller range: + // stores past it are dropped and loads return zero. Forget what the shadow + // claims for this id so the next SyncBufferBindingPoints issues the bind for + // real. Only when the extent actually moved: an orphaning respecify at the same + // size is Minecraft's per-frame hot path and its bindings are still exact. + if (extentChanged) { + InvalidateIndexedBufferBindingShadowsForId(resource.id); + } } Bool StorageMatches(const GLESBufferResource& resource, const BufferObject& bufferObject) { @@ -1180,6 +1202,12 @@ namespace MobileGL::MG_Backend::DirectGLES { GLintptr offset = 0; GLsizeiptr size = 0; Bool isBase = true; + // False when the driver's binding at this point is no longer described by the + // fields above and the next bind must be issued whatever it asks for. Set by + // InvalidateIndexedBufferBindingShadowsForId after a store was re-specified at + // a new size: the id is still bound, so the entry must NOT be scrubbed to + // base(0) (a later bind of 0 would then be false-skipped) - only distrusted. + Bool known = true; }; constexpr SizeT kMaxIndexedBufferBindings = 64; IndexedBufferBinding g_indexedUBOBindings[kMaxIndexedBufferBindings]; @@ -1211,20 +1239,30 @@ namespace MobileGL::MG_Backend::DirectGLES { g_boundPixelUnpackBufferId = 0; } } + + void InvalidateIndexedBufferBindingShadowsForId(Uint id) { + if (id == 0) return; + for (auto& binding : g_indexedUBOBindings) { + if (binding.id == id) binding.known = false; + } + for (auto& binding : g_indexedSSBOBindings) { + if (binding.id == id) binding.known = false; + } + } } // namespace void BindBufferBaseCached(GLenum glTarget, Uint index, Uint id) { auto* s = IndexedBindingShadow(glTarget, index); - if (s && s->isBase && s->id == id) return; + if (s && s->known && s->isBase && s->id == id) return; g_GLESFuncs.glBindBufferBase(glTarget, index, id); - if (s) *s = {id, 0, 0, true}; + if (s) *s = {id, 0, 0, true, true}; } void BindBufferRangeCached(GLenum glTarget, Uint index, Uint id, GLintptr offset, GLsizeiptr size) { auto* s = IndexedBindingShadow(glTarget, index); - if (s && !s->isBase && s->id == id && s->offset == offset && s->size == size) return; + if (s && s->known && !s->isBase && s->id == id && s->offset == offset && s->size == size) return; g_GLESFuncs.glBindBufferRange(glTarget, index, id, offset, size); - if (s) *s = {id, offset, size, false}; + if (s) *s = {id, offset, size, false, true}; } void InvalidateIndexedBufferBindingCache() { @@ -1548,6 +1586,25 @@ namespace MobileGL::MG_Backend::DirectGLES { const SizeT componentSize = GetDataTypeSize(type); return componentSize == 0 ? 0 : componentSize * static_cast(size); } + + // Deinterleaves elementCount elements of componentCount doubles into a tightly packed + // float32 stream - the fetch half of the fp64 demotion the shader side already does + // unconditionally (DemoteFloat64Pass). GL byte strides and offsets are arbitrary, so + // no component carries an 8-byte alignment guarantee and each is copied out before it + // is narrowed. + void NarrowDoubleStreamToFloat32(const Uint8* sourceBase, SizeT sourceStride, SizeT componentCount, + SizeT elementCount, Vector& outData) { + outData.resize(elementCount * componentCount); + for (SizeT element = 0; element < elementCount; ++element) { + const Uint8* sourceElement = sourceBase + element * sourceStride; + Float* destinationElement = outData.data() + element * componentCount; + for (SizeT component = 0; component < componentCount; ++component) { + Double value = 0.0; + Memcpy(&value, sourceElement + component * sizeof(Double), sizeof(Double)); + destinationElement[component] = static_cast(value); + } + } + } } // namespace BackendVertexArrayObject::BackendVertexArrayObject() { @@ -1555,6 +1612,7 @@ namespace MobileGL::MG_Backend::DirectGLES { ZoneScopedC(TRACY_ZONECOLOR_BACKEND); #endif m_clientAttributeBufferIds.fill(0); + m_convertedAttributeBufferIds.fill(0); g_GLESFuncs.glGenVertexArrays(1, &m_backendVAOId); if (m_backendVAOId == 0) { MGLOG_E_ONCE("Failed to generate vertex array object."); @@ -1580,6 +1638,13 @@ namespace MobileGL::MG_Backend::DirectGLES { bufferId = 0; } } + for (auto& bufferId : m_convertedAttributeBufferIds) { + if (bufferId != 0) { + BufferImpl::NoteBufferIdDeleted(bufferId); + g_GLESFuncs.glDeleteBuffers(1, &bufferId); + bufferId = 0; + } + } } namespace { @@ -1748,10 +1813,15 @@ namespace MobileGL::MG_Backend::DirectGLES { // that never calls a *BaseInstance entry point never pays for this compare. const Uint32 fetchBaseInstance = g_pendingFetchBaseInstance; const Bool baseInstanceDirty = m_syncedFetchBaseInstance != fetchBaseInstance; - const Bool emitAttributes = attributesDirty || baseInstanceDirty; + // A narrowed GL_DOUBLE stream is derived from the source buffer's CONTENT, and no + // VAO version moves when an app writes into a buffer, so the version gate cannot + // prove such a stream is still current. Re-walk while one is live; the walk itself + // re-checks the buffer's change serial and only re-converts on a real move. + const Bool emitAttributes = attributesDirty || baseInstanceDirty || m_hasConvertedFloat64Attribute; if (!emitAttributes && !indexBufferDirty) { return; } + m_hasConvertedFloat64Attribute = false; Bind(); @@ -1777,35 +1847,51 @@ namespace MobileGL::MG_Backend::DirectGLES { m_syncedAttributeVersions[attribIndex].FormatVersion; Bool needsSyncBuffer = bufferIdsRemitted || allAttributeVersions[attribIndex].BufferVersion != m_syncedAttributeVersions[attribIndex].BufferVersion; - if (!needsSyncFormat && !needsSyncBuffer && !needsSyncBaseInstance) continue; - - // This is where a 64-bit array actually stops. glVertexAttribLFormat is a legal call - // in a GL 4.3 context and the frontend RECORDS its format (the state queries have to - // answer), so IsLong does arrive here - what this backend cannot do is FEED it: - // SupportsFloat64VertexAttributes is false because ES has no GL_DOUBLE vertex format - // and ESSL has no fp64 type, and passing GL_DOUBLE to glVertexAttribPointer would - // only raise GL_INVALID_ENUM on the real driver. Disabling rather than merely - // skipping matters: becoming long bumps FormatVersion, not SwitchVersion, so the - // enable/disable block above will not run again and an already-enabled array would - // stay enabled with no pointer and no ARRAY_BUFFER binding - which ES 3.1+ makes an - // INVALID_OPERATION at draw. + // This is where a 64-bit array is narrowed. glVertexAttribLFormat is a legal call in + // a GL 4.3 context and the frontend RECORDS its format (the state queries have to + // answer), so IsLong does arrive here - what this backend cannot do is feed it at + // full precision: ES has no GL_DOUBLE vertex FORMAT and ESSL has no fp64 type, and + // passing GL_DOUBLE to glVertexAttribPointer would only raise GL_INVALID_ENUM on the + // real driver. But the FORMAT is the only 64-bit thing here: the source bytes are + // ordinary IEEE-754 doubles, and MobileGL already narrows every fp64 value in every + // shader (DemoteFloat64Pass) and every glUniform*d the same way, so the array is + // deinterleaved into a float32 stream and fetched as GL_FLOAT rather than dropped. + // glVertexAttribFormat(GL_DOUBLE) asks for exactly that conversion anyway; the L form + // asks for more precision than any backend here can give, and gets the same stream. // - // IsLong is not the only way a 64-bit array gets here: glVertexAttribFormat - // with GL_DOUBLE asks for doubles in memory CONVERTED to float, so it is not - // long, is not declined by the frontend, and still has no ES vertex format. - // Leaving that one enabled did not merely raise INVALID_ENUM - the Adreno - // driver dereferenced null inside the next draw and took the process with it - // (SIGSEGV in libGLESv2_adreno, KHR-GL43.vertex_attrib_binding.basic-input-case4), - // because the array stayed enabled with no pointer the failed call could set. - // The type test therefore covers the storage, not the spelling. + // The conversion is deliberately NOT behind the version gate below: it is derived + // from buffer CONTENT, which no VAO version covers. Its own memo (keyed on the + // source buffer's change serial) is what keeps the repeat cost down. + // + // When no stream can be built the array is DISABLED rather than left alone. That + // matters: becoming long bumps FormatVersion, not SwitchVersion, so the + // enable/disable block above will not run again and an already-enabled array would + // stay enabled with no pointer and no ARRAY_BUFFER binding - which did not merely + // raise INVALID_ENUM but had the Adreno driver dereference null inside the next draw + // and take the process with it (SIGSEGV in libGLESv2_adreno, + // KHR-GL43.vertex_attrib_binding.basic-input-case4). if (attrib.IsLong || attrib.Type == DataType::Float64) { - MGLOG_W_ONCE("DirectGLES: vertex attribute %u is a 64-bit (GL_DOUBLE) array, which this " - "backend cannot feed - disabling the array", - attribIndex); + if (attrib.Enabled && attrib.Type == DataType::Float64 && + SyncFloat64AttributeAsFloat32(attribIndex, attrib, fetchBaseInstance)) { + m_hasConvertedFloat64Attribute = true; + // Explicit, not redundant: an earlier walk that could not build the stream + // disabled this array, and becoming feedable again bumps no SwitchVersion, + // so the enable/disable block above would never turn it back on. + g_GLESFuncs.glEnableVertexAttribArray(attribIndex); + g_GLESFuncs.glVertexAttribDivisor(attribIndex, attrib.Divisor); + continue; + } + if (attrib.Enabled) { + MGLOG_W_ONCE("DirectGLES: vertex attribute %u is a 64-bit (GL_DOUBLE) array whose source " + "stream could not be narrowed to float32 - disabling the array", + attribIndex); + } g_GLESFuncs.glDisableVertexAttribArray(attribIndex); continue; } + if (!needsSyncFormat && !needsSyncBuffer && !needsSyncBaseInstance) continue; + // A resolved stride of zero is the binding model's "never advance" (see // VertexAttribute::Stride) and glVertexAttribPointer cannot say it - a zero // stride argument there means "tightly packed" instead, i.e. exactly the @@ -1928,11 +2014,50 @@ namespace MobileGL::MG_Backend::DirectGLES { continue; } - // Same reason as SyncToBackend, including why the test is on the storage rather - // than on IsLong: there is no ES vertex format for a 64-bit array, and this path - // only ever reaches glVertexAttribPointer/IPointer. + // Same narrowing as SyncToBackend (the long note lives there), with the draw's own + // fetch range standing in for the buffer extent a client array does not have. The + // upload starts at element 0 so `first` keeps indexing the stream, exactly as the + // unconverted upload below does. The test is on the storage rather than on IsLong + // because glVertexAttribFormat(GL_DOUBLE) is 64-bit data without being long. if (attrib.IsLong || attrib.Type == DataType::Float64) { - g_GLESFuncs.glDisableVertexAttribArray(attribIndex); + const auto* sourceBase = reinterpret_cast(attrib.Offset); + if (attrib.Type != DataType::Float64 || sourceBase == nullptr || attrib.Size < 1 || + attrib.Size > 4) { + g_GLESFuncs.glDisableVertexAttribArray(attribIndex); + continue; + } + + auto& bufferId = m_clientAttributeBufferIds[attribIndex]; + if (bufferId == 0) { + g_GLESFuncs.glGenBuffers(1, &bufferId); + if (bufferId == 0) { + MGLOG_E_ONCE("Failed to create client-side vertex attribute upload buffer."); + g_GLESFuncs.glDisableVertexAttribArray(attribIndex); + continue; + } + } + + const SizeT componentCount = static_cast(attrib.Size); + const SizeT sourceElementSize = componentCount * sizeof(Double); + // A client array only ever reaches here through a pointer call, whose stride 0 + // the frontend already resolved to the element size, so this is a guard rather + // than a case (VertexAttribute::Stride). + const SizeT sourceStride = + attrib.Stride > 0 ? static_cast(attrib.Stride) : sourceElementSize; + const SizeT elementCount = static_cast(first) + static_cast(count); + Vector converted; + NarrowDoubleStreamToFloat32(sourceBase, sourceStride, componentCount, elementCount, converted); + + BufferImpl::BindBufferId(GL_ARRAY_BUFFER, bufferId); + g_GLESFuncs.glBufferData(GL_ARRAY_BUFFER, + static_cast(converted.size() * sizeof(Float)), + converted.data(), GL_STREAM_DRAW); + // GL ignores `normalized` for floating-point array types, so it is not + // forwarded here either. + g_GLESFuncs.glVertexAttribPointer(attribIndex, attrib.Size, GL_FLOAT, GL_FALSE, + static_cast(componentCount * sizeof(Float)), + nullptr); + g_GLESFuncs.glEnableVertexAttribArray(attribIndex); continue; } @@ -1972,6 +2097,120 @@ namespace MobileGL::MG_Backend::DirectGLES { } + Bool BackendVertexArrayObject::SyncFloat64AttributeAsFloat32( + Uint attribIndex, const MG_State::GLState::VertexAttribute& attrib, Uint32 fetchBaseInstance) { +#ifdef TRACY_ENABLE + ZoneScopedC(TRACY_ZONECOLOR_BACKEND); +#endif + if (attribIndex >= m_convertedAttributeBufferIds.size() || attrib.Size < 1 || attrib.Size > 4) { + return false; + } + const auto& bufferObject = attrib.Buffer; + if (!bufferObject) { + // A client-memory 64-bit array is narrowed on the draw path instead, which is the + // only place its fetch range is known (SyncClientSideAttributesForDrawArrays). + return false; + } + + // The frontend shadow is what the conversion reads, so a shader write that has not + // been pulled back yet has to land first. A no-op unless one is outstanding. + bufferObject->SyncGpuWrites(); + const Uint8* const sourceBase = bufferObject->MappedData(); + const SizeT sourceSize = bufferObject->GetSize(); + if (sourceBase == nullptr || attrib.Offset >= sourceSize) { + return false; + } + + const SizeT componentCount = static_cast(attrib.Size); + const SizeT sourceElementSize = componentCount * sizeof(Double); + const SizeT available = sourceSize - attrib.Offset; + if (available < sourceElementSize) { + return false; + } + + // A resolved stride of zero is the binding model's "never advance" (see + // VertexAttribute::Stride): exactly one element exists and every vertex reads it, so + // exactly one is converted. Otherwise the array's extent is the source buffer's own - + // SyncToBackend has no draw range, and a whole-array conversion is affordable because + // it is memoised on the buffer's change serial and 64-bit arrays are vanishingly rare. + const Bool neverAdvances = attrib.Stride <= 0; + const SizeT sourceStride = neverAdvances ? sourceElementSize : static_cast(attrib.Stride); + const SizeT elementCount = neverAdvances ? 1 : ((available - sourceElementSize) / sourceStride) + 1; + + // baseInstance shifts the ELEMENT index of a divisor'd array, and one element of the + // converted stream is componentCount floats. A zero stride never advances, so no + // shift can move it. A shift past the array's own extent has no source data at all. + const SizeT firstElement = (fetchBaseInstance != 0 && attrib.Divisor != 0 && !neverAdvances) + ? static_cast(fetchBaseInstance) + : 0; + if (firstElement >= elementCount) { + return false; + } + + auto& stream = m_convertedAttributeStreams[attribIndex]; + Uint& convertedBufferId = m_convertedAttributeBufferIds[attribIndex]; + const Uint64 sourceLifetimeId = bufferObject->GetLifetimeId(); + const Uint64 sourceChangeSerial = bufferObject->GetChangeSerial(); + // A persistent map is written through the pointer, with no API call to bump the change + // serial (see BufferObject::SyncPersistentMappedRange), so its serial cannot prove the + // converted copy is still current and the memo is never trusted for one. + const Bool memoHit = + stream.valid && convertedBufferId != 0 && !bufferObject->IsBackendPersistentMapped() && + stream.sourceLifetimeId == sourceLifetimeId && stream.sourceChangeSerial == sourceChangeSerial && + stream.sourceOffset == attrib.Offset && stream.sourceStride == sourceStride && + stream.componentCount == componentCount && stream.elementCount == elementCount; + if (!memoHit) { + if (convertedBufferId == 0) { + g_GLESFuncs.glGenBuffers(1, &convertedBufferId); + if (convertedBufferId == 0) { + MGLOG_E_ONCE("Failed to create the float32 scratch buffer for the 64-bit vertex array at " + "attribute %u.", + attribIndex); + return false; + } + } + Vector converted; + NarrowDoubleStreamToFloat32(sourceBase + attrib.Offset, sourceStride, componentCount, elementCount, + converted); + BufferImpl::BindBufferId(GL_ARRAY_BUFFER, convertedBufferId); + g_GLESFuncs.glBufferData(GL_ARRAY_BUFFER, + static_cast(converted.size() * sizeof(Float)), + converted.data(), GL_STREAM_DRAW); + stream.valid = true; + stream.sourceLifetimeId = sourceLifetimeId; + stream.sourceChangeSerial = sourceChangeSerial; + stream.sourceOffset = attrib.Offset; + stream.sourceStride = sourceStride; + stream.componentCount = componentCount; + stream.elementCount = elementCount; + MGLOG_D("DirectGLES: narrowed the 64-bit vertex array at attribute %u to %zu float32 element(s).", + attribIndex, elementCount); + } + + const SizeT convertedElementSize = componentCount * sizeof(Float); + if (neverAdvances) { + // Only the binding-point API can say "stride 0": glVertexAttribPointer's zero + // means "tightly packed" instead, i.e. the opposite, and would walk the driver + // straight off the end of the single converted element. + if (!HasVertexBindingApi()) { + return false; + } + g_GLESFuncs.glVertexAttribFormat(attribIndex, attrib.Size, GL_FLOAT, GL_FALSE, 0); + g_GLESFuncs.glVertexAttribBinding(attribIndex, attribIndex); + g_GLESFuncs.glBindVertexBuffer(attribIndex, convertedBufferId, 0, 0); + return true; + } + + BufferImpl::BindBufferId(GL_ARRAY_BUFFER, convertedBufferId); + // `normalized` is deliberately GL_FALSE rather than attrib.Normalized: GL ignores it + // for floating-point array types, and honouring it would scale the fetched values + // (KHR-GL43.vertex_attrib_binding.basic-input-case5 passes GL_TRUE and expects 10/20). + g_GLESFuncs.glVertexAttribPointer(attribIndex, attrib.Size, GL_FLOAT, GL_FALSE, + static_cast(convertedElementSize), + (const void*)(firstElement * convertedElementSize)); + return true; + } + StateBackendObjectRegistry g_backendVertexArrayObjects; } // namespace VertexArrayImpl @@ -4764,6 +5003,22 @@ namespace MobileGL::MG_Backend::DirectGLES { return reflectionName; } + // GL_MAX__IMAGE_UNIFORMS as the ES driver reports it, which is also exactly what + // MobileGL advertises for it (GL_Getter answers from the same DynamicBackendParameters). + // -1 for a stage the ES side has no such limit for, which is the "cannot say" answer + // the diagnostic that reads it prints rather than a made-up number. [[maybe_unused]] + // because its only caller is an MGLOG_E argument, and MGLOG_E compiles to nothing in a + // build whose MOBILEGL_LOG_ACTIVE_LEVEL is above ERROR. + [[maybe_unused]] Int AdvertisedStageImageUniformLimit(ShaderStage stage) { + switch (stage) { + case ShaderStage::Vertex: return g_GLESCapabilities.MaxVertexImageUniforms; + case ShaderStage::Geometry: return g_GLESCapabilities.MaxGeometryImageUniforms; + case ShaderStage::Fragment: return g_GLESCapabilities.MaxFragmentImageUniforms; + case ShaderStage::Compute: return g_GLESCapabilities.MaxComputeImageUniforms; + default: return -1; + } + } + // Whether a glslang layout format is one GLSL ES has in core; the rest reach ES only // through GL_NV_image_formats. Asked of DECLARED formats, which this backend passes // through untouched - the emitted ESSL still has to be legal for the driver. @@ -5206,6 +5461,50 @@ namespace MobileGL::MG_Backend::DirectGLES { effectiveSpirv = &outputIndexSpirv; } + // Same rule, different resource, every stage: GL 4.3 lets an array of storage + // blocks be indexed with any dynamically-uniform expression, GLSL ES keeps the + // ES 3.1 constant-expression rule, and the Qualcomm compiler enforces it + // ("indexing into an SSBO array using a non-constant expression is not + // permitted") - losing the stage, the program, and every dispatch that used + // it, while the frontend keeps reporting the link glslang performed. Fold or + // lower the index here, on the ESSL path only: the same module is legal for + // DirectVulkan, which binds the array as one descriptor array. + // + // NO KEY MATERIAL, and that is a conclusion rather than an omission: this takes the + // module and nothing else - no capability bit arms it, no per-program plan steers + // it - and it self-gates on the module's own content + // (BinaryHasDynamicStorageBlockArrayIndexing). The module is already the largest + // thing in the L2 key, so it is fully covered. Contrast LowerViewportIndexForEssl, + // whose signature is equally module-only but which SupportsViewportArray ARMS - + // that bit is in the key precisely because of it. + Vector blockArrayIndexSpirv; + if (MG_Util::ShaderTranspiler::ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl( + *effectiveSpirv, blockArrayIndexSpirv, enableSpirvValidation) && + !blockArrayIndexSpirv.empty()) { + effectiveSpirv = &blockArrayIndexSpirv; + } + + // glslang kept the application's layout(offset = N) on the atomic counters it + // lowered onto gl_AtomicCounterBlock_, and no std140/std430 layout can put + // member 0 anywhere but offset 0 - so SPIRV-Cross throws ("cannot be expressed as + // neither std430 nor std140") and the stage never reaches the driver. Collapse the + // block into one uint array at offset 0 and re-index each counter to the element + // that used to be at its byte offset; the buffer then stays bound whole, which it + // has to (GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT is 32 on this device, so an + // 8-byte bind offset is not expressible). BEFORE SetAtomicCounterBlockBindings + // below, which only moves the block's BINDING and needs the block intact. + // + // NO KEY MATERIAL either, for the same reason - and note where the application's + // layout(offset = N) values live: glslang already baked them into the module as + // member Offset decorations, so they are in the key as MODULE BYTES. There is no + // separate offset input to carry. + Vector atomicCounterSpirv; + if (MG_Util::ShaderTranspiler::ShaderCompiler::FlattenAtomicCounterBlockOffsetsForEssl( + *effectiveSpirv, atomicCounterSpirv, enableSpirvValidation) && + !atomicCounterSpirv.empty()) { + effectiveSpirv = &atomicCounterSpirv; + } + MG_Util::ShaderTranspiler::SpvcSession spvcSession(*effectiveSpirv, MG_Util::ShaderTranspiler::SessionUsageBit::Transpile); @@ -5371,6 +5670,18 @@ namespace MobileGL::MG_Backend::DirectGLES { } } std::set flattenedXfbBlockNames; + // Stages whose ESSL had a read+write image declaration doubled into a coherent + // read/write pair, and by how many. Empty for every program but a handful; consulted + // ONLY when the link then fails, because the doubling spends the driver's per-stage + // GL_MAX_*_IMAGE_UNIFORMS budget that MobileGL keeps advertising unadjusted (halving + // the advertised value would fail basic-api and NotSupported-out cases that never pay + // the doubling, so the limit must stay honest and the connection has to be made here + // instead). See the budget note on SplitReadWriteImageUniforms. + struct SplitImageUniformStage { + ShaderStage stage; + Uint splitCount; + }; + Vector splitImageUniformStages; // Desktop GLSL keeps SEPARATE name namespaces for input and output interface // blocks, so ONE stage may legally declare `in FOO {...}` and `out FOO {...}` at @@ -5642,7 +5953,11 @@ namespace MobileGL::MG_Backend::DirectGLES { // declaration and preserves its binding - an image unit cannot be set from // the API in ES, so the qualifier is the only binding mechanism there is, // and both halves of the pair have to still be carrying theirs when it runs. - source = SplitReadWriteImageUniforms(source); + Uint splitImageUniformCount = 0; + source = SplitReadWriteImageUniforms(source, &splitImageUniformCount); + if (splitImageUniformCount != 0) { + splitImageUniformStages.push_back({shader->GetShaderStage(), splitImageUniformCount}); + } source = RemoveLayoutBinding(source); source = ProcessOutColorLocations(source); source = ForceFlatIntegerVaryings(source, glShaderType); @@ -5787,6 +6102,25 @@ namespace MobileGL::MG_Backend::DirectGLES { // in an INFO-level artifact. MGLOG_E("Program linking failed. State program ID: %u, backend program ID: %u, driver log: %s", stateProgramObject->GetExternalIndex(), m_backendProgramId, log.data()); + // The one link failure MobileGL can name a cause for that the driver's log never + // will: ESSL has no legal single declaration for a read+write image outside + // r32f/r32i/r32ui, so those are split into a coherent pair and the stage ends up + // declaring more image uniforms than the application did - against a + // GL_MAX_*_IMAGE_UNIFORMS that is still the driver's raw number, because lowering + // it would fail basic-api and NotSupported-out every case that only ever uses + // readonly/writeonly images. A shader declaring more than half a stage's budget in + // read+write images therefore links here and nowhere else, and without this line + // the next reader has only a generic driver message to go on. + for (const SplitImageUniformStage& split : splitImageUniformStages) { + MGLOG_E("...and %u read+write image uniform(s) in stage %s were split into coherent " + "read/write pairs, so that stage declares %u image uniform(s) more than the " + "program did; GL_MAX_*_IMAGE_UNIFORMS for it is %d. If the driver log names " + "image uniforms, that is the cause.", + split.splitCount, + MG_Util::ConvertGLEnumToString( + MG_Util::ConvertShaderStageToGLEnum(split.stage)).c_str(), + split.splitCount, AdvertisedStageImageUniformLimit(split.stage)); + } } else { MGLOG_D("Program linked successfully. ID: %u", m_backendProgramId); } diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index 708698a5..a289a242 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -509,10 +509,44 @@ namespace MobileGL::MG_Backend::DirectGLES { PendingAttribValueMask& GetPendingAttribValueMaskMemo() { return m_pendingAttribValueMask; } private: + // Narrows one enabled GL_DOUBLE array into a tightly packed float32 stream held in + // this VAO's own scratch buffer and declares the attribute against it. ES has no + // 64-bit vertex format, but the source bytes are ordinary IEEE-754 doubles and every + // fp64 value in every shader is already narrowed to 32 bits (DemoteFloat64Pass), so + // narrowing the ARRAY is the coherent completion of that decision rather than + // dropping it. Returns false when the stream cannot be built, in which case the + // caller must DISABLE the array - leaving a 64-bit array enabled with no pointer is + // what the Adreno driver turns into a SIGSEGV at the next draw. + Bool SyncFloat64AttributeAsFloat32(Uint attribIndex, const MG_State::GLState::VertexAttribute& attrib, + Uint32 fetchBaseInstance); + + // What the converted float32 stream in m_convertedAttributeBufferIds[i] was built + // from. A hit skips the CPU conversion and the re-upload; the buffer's change serial + // is part of the key, so a glBufferSubData into the source invalidates it. + struct ConvertedFloat64Stream { + Bool valid = false; + Uint64 sourceLifetimeId = 0; + Uint64 sourceChangeSerial = 0; + SizeT sourceOffset = 0; + SizeT sourceStride = 0; + SizeT componentCount = 0; + SizeT elementCount = 0; + }; + ResolvedDrawBuffers m_resolvedDrawBuffers; PendingAttribValueMask m_pendingAttribValueMask; Uint m_backendVAOId = 0; Array m_clientAttributeBufferIds; + // Scratch stores for the buffer-backed GL_DOUBLE narrowing. Deliberately separate + // from m_clientAttributeBufferIds: that one holds the per-draw upload of a + // CLIENT-MEMORY array, and an attribute index can carry both shapes over its life. + Array m_convertedAttributeBufferIds; + Array + m_convertedAttributeStreams; + // True while at least one attribute of this VAO is fed by a converted stream. Such a + // stream is derived from buffer CONTENT, which no VAO version covers, so the config + // version early-out in SyncToBackend must not be trusted while it is set. + Bool m_hasConvertedFloat64Attribute = false; Bool m_isInitialized = false; Uint16 m_syncedIndexBufferVersion = 0; // Identity of the buffer the version above was stamped against. Raw and never diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.cpp b/MobileGL/MG_Backend/DirectGLES/Utils.cpp index b262539b..a6a6bd8d 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Utils.cpp @@ -882,12 +882,35 @@ namespace MobileGL::MG_Backend::DirectGLES { SizeT length; String text; }; + + // The offset just past the `;` that terminates the call whose argument list opens at + // `openParen`, or npos when what follows is not a plain statement. Parentheses alone + // are counted: every other bracket a GLSL argument list can contain is balanced + // inside them, and imageStore returns void, so a well-formed call site is always + // `imageStore(...);` and anything else is a shape this pass declines to edit. + SizeT FindEndOfCallStatement(const String& code, SizeT openParen) { + Int depth = 0; + SizeT scan = openParen; + for (; scan < code.size(); ++scan) { + if (code[scan] == '(') { + ++depth; + } else if (code[scan] == ')' && --depth == 0) { + break; + } + } + if (scan >= code.size()) return String::npos; + const SizeT after = code.find_first_not_of(" \t\r\n", scan + 1); + if (after == String::npos || code[after] != ';') return String::npos; + return after + 1; + } } // namespace - String SplitReadWriteImageUniforms(const String& glslCode) { + String SplitReadWriteImageUniforms(const String& glslCode, Uint* outSplitCount) { #ifdef TRACY_ENABLE ZoneScopedC(TRACY_ZONECOLOR_BACKEND); #endif + // Written before any early return, so the caller never reads a stale count. + if (outSplitCount != nullptr) *outSplitCount = 0; if (glslCode.find("image") == String::npos) { return glslCode; } @@ -949,6 +972,7 @@ namespace MobileGL::MG_Backend::DirectGLES { SizeT declIndex; SizeT start; SizeT length; + SizeT callOpen; // the '(' of the call this argument belongs to }; Vector storeSites; for (SizeT pos = glslCode.find("image"); pos != String::npos; pos = glslCode.find("image", pos + 1)) { @@ -998,7 +1022,7 @@ namespace MobileGL::MG_Backend::DirectGLES { break; case ImageBuiltinAccess::Store: decl.stored = true; - storeSites.push_back({declIndex, argStart, argEnd - argStart}); + storeSites.push_back({declIndex, argStart, argEnd - argStart, openParen}); break; case ImageBuiltinAccess::None: break; @@ -1024,6 +1048,7 @@ namespace MobileGL::MG_Backend::DirectGLES { decl.writeName = MakeImageWriteAliasName(decl.name, glslCode, takenAliases); takenAliases.push_back(decl.writeName); decl.split = true; + if (outSplitCount != nullptr) ++*outSplitCount; // Both halves carry `coherent`; see BuildImageDeclaration. The // single-declaration cases below stay as they were - nothing aliases them, so // there is no visibility to restore and no reason to pay for the cache @@ -1047,6 +1072,24 @@ namespace MobileGL::MG_Backend::DirectGLES { const ImageUniformDecl& decl = decls[site.declIndex]; if (!decl.split) continue; edits.push_back({site.start, site.length, decl.writeName}); + // ...and an explicit barrier behind it. `coherent` on both halves is what makes + // the store VISIBLE to a load through the other variable, but it says nothing + // about ORDER within one invocation - and the whole reason a declaration is split + // is that the shader both stores and loads through it, which on the ES side is now + // a write to one variable followed by a read of another the compiler has no reason + // to believe alias. Adreno duly serves the load from before the store + // (KHR-GL4x.shader_image_load_store.advanced-memory-order's store/load/compare + // loop reads back the previous iteration's value). memoryBarrierImage() is the + // GLSL primitive for exactly that ordering, is core GLSL ES 3.10 in every stage, + // and is not an execution barrier, so it is legal in non-uniform control flow too. + // + // Confined to the split pair: a single-declaration repair has nothing aliasing it + // and must not pay for this, and a shader that never got split never sees it at + // all. + const SizeT statementEnd = FindEndOfCallStatement(glslCode, site.callOpen); + if (statementEnd != String::npos) { + edits.push_back({statementEnd, 0, " memoryBarrierImage();"}); + } } if (edits.empty()) { return glslCode; diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.h b/MobileGL/MG_Backend/DirectGLES/Utils.h index 063651d2..e4712ad1 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.h +++ b/MobileGL/MG_Backend/DirectGLES/Utils.h @@ -197,8 +197,9 @@ namespace MobileGL::MG_Backend::DirectGLES { // * stored only -> add `writeonly` // * both -> emit TWO declarations on the same binding and of the // same type, `coherent readonly ` and `coherent - // writeonly `, and point - // every imageStore at the second one. Several image + // writeonly `, point + // every imageStore at the second one, and follow each of + // those stores with `memoryBarrierImage();`. Several image // variables may share an image unit as long as they have // the same type and format, which is exactly what the pair // is. @@ -209,6 +210,15 @@ namespace MobileGL::MG_Backend::DirectGLES { // read-after-write cross-variable. The single-declaration repairs above do not get it - // nothing aliases them. // + // The barrier is the other half of the same problem, and coherent alone did not cover it: + // visibility is not ORDER. Within one invocation the ES compiler sees a write to one + // variable and a read of another it has no reason to believe alias, and is free to serve + // the read from before the write - which is what advanced-memory-order's store/load/ + // compare loop measured on Adreno. memoryBarrierImage() orders exactly those two, is core + // GLSL ES 3.10 in every stage, and is not an execution barrier, so it is legal in + // non-uniform control flow. It costs something in a shader that stores to a read+write + // image in a loop, which is why it is confined to the split pair. + // // Budget note: the split DOUBLES the image-uniform count of the stage it fires in, so // a driver advertising a tight GL_MAX_{FRAGMENT,VERTEX,...}_IMAGE_UNIFORMS can turn a // shader that used to compile into a link failure. ES only guarantees 4 fragment image @@ -218,7 +228,12 @@ namespace MobileGL::MG_Backend::DirectGLES { // Runs on the transpiled ESSL, so it must see the bindings the frontend units were // already rewritten to and must run before those bindings are stripped - see the call // site in Managers.cpp. - String SplitReadWriteImageUniforms(const String& glslCode); + // + // `outSplitCount`, when given, receives the number of declarations that were actually + // doubled - i.e. exactly how many image uniforms this stage gained over what the + // application declared. Zero for every shader but a handful, and the only number the + // budget note above can be reported with. + String SplitReadWriteImageUniforms(const String& glslCode, Uint* outSplitCount = nullptr); // Prefix of the per-sampler float uniform that carries GL_TEXTURE_LOD_BIAS into // the shader (see EmulateTextureLodBias); the suffix is the sampler's own name. constexpr const char* LOD_BIAS_UNIFORM_PREFIX = "mg_lodBias_"; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp index c69a994a..713aad1f 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp @@ -108,8 +108,32 @@ namespace MobileGL::MG_Backend::DirectVulkan { continue; } - const VkFormat sourceVkFormat = + VkFormat sourceVkFormat = ToVkVertexFormat(attr.Type, attr.Size, attr.Normalized, attr.IsInteger, attr.IsBgra, attr.IsLong); + VertexStreamConversion conversion = VertexStreamConversion::None; + // Gated on the SAME flag ToVkVertexFormat gates its 64-bit path on, and that is + // load-bearing rather than belt-and-braces: the narrowing is only correct because + // DemoteFloat64Pass already turned the shader's `dvec` input into a `vec`, and that + // pass runs precisely when the backend declares no 64-bit vertex support. With the + // flag set, a dvec3/dvec4 is declined by ToVkVertexFormat AND left 64-bit in the + // module, so a float32 stream would be fed to a Float64 input. + const Bool narrowFloat64Arrays = + MG_Backend::pActiveBackendObject == nullptr || + !MG_Backend::pActiveBackendObject->GetDynamicParameters().SupportsFloat64VertexAttributes; + if (sourceVkFormat == VK_FORMAT_UNDEFINED && attr.Type == DataType::Float64 && narrowFloat64Arrays) { + // No native 64-bit fetch here (see ToVkVertexFormat's Float64 case), but the + // source bytes are ordinary IEEE-754 doubles and DemoteFloat64Pass has already + // narrowed every dvec input to a vec, so the array is narrowed to match rather + // than dropped. Mirrors what DirectGLES does for the same state. + const VkFormat narrowedFormat = ToFloat32VertexFormat(attr.Size); + if (narrowedFormat != VK_FORMAT_UNDEFINED && SupportsVertexBufferFormat(narrowedFormat)) { + sourceVkFormat = narrowedFormat; + conversion = VertexStreamConversion::Float64ToFloat32; + MGLOG_W_ONCE("Vertex attribute location=%u is a 64-bit (GL_DOUBLE) array; fetching it at " + "float32 precision through format=%d (size=%d long=%s)", + location, static_cast(narrowedFormat), attr.Size, attr.IsLong ? "true" : "false"); + } + } if (sourceVkFormat == VK_FORMAT_UNDEFINED) { MGLOG_E_ONCE("Unsupported vertex attribute layout (location=%u, type=%s, size=%d): the array is " "enabled but cannot be mapped to a VkFormat", @@ -119,8 +143,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { } VkFormat vkFormat = sourceVkFormat; - VertexStreamConversion conversion = VertexStreamConversion::None; - if (!SupportsVertexBufferFormat(vkFormat)) { + if (conversion == VertexStreamConversion::None && !SupportsVertexBufferFormat(vkFormat)) { if (IsScaledIntegerVertexFormat(vkFormat)) { const VkFormat fallbackFormat = ToFloat32VertexFormat(attr.Size); if (fallbackFormat != VK_FORMAT_UNDEFINED && SupportsVertexBufferFormat(fallbackFormat)) { @@ -189,7 +212,8 @@ namespace MobileGL::MG_Backend::DirectVulkan { if (sourceStride != 0) { if (conversion == VertexStreamConversion::Repack) { stride = static_cast(attribByteSize); - } else if (conversion == VertexStreamConversion::ScaledIntegerToFloat32) { + } else if (conversion == VertexStreamConversion::ScaledIntegerToFloat32 || + conversion == VertexStreamConversion::Float64ToFloat32) { stride = static_cast(attr.Size * static_cast(sizeof(Float))); } } @@ -336,11 +360,11 @@ namespace MobileGL::MG_Backend::DirectVulkan { // no 64-bit vertex attribute support: DemoteFloat64Pass has already narrowed every // `dvec` input to a `vec` by then, so PackDoubleVertexInputsPass finds nothing to pack // and a UINT-formatted attribute would be fed to a float input - garbage with no - // diagnostic anywhere. Declining here drops the array instead (the caller skips - // UNDEFINED attributes and reports them through unsupportedAttribMask), which is what - // DirectGLES does for the same state. The frontend RECORDS the format either way, so - // this gate is the only thing standing between a legal glVertexAttribLFormat and a - // mismatched pipeline. + // diagnostic anywhere. Declining here hands the attribute to the caller's + // Float64ToFloat32 fallback instead, which narrows the source doubles to match the + // demoted `vec` input - the same thing DirectGLES does for the same state. The + // frontend RECORDS the format either way, so this gate is the only thing standing + // between a legal glVertexAttribLFormat and a mismatched pipeline. if (MG_Backend::pActiveBackendObject == nullptr || !MG_Backend::pActiveBackendObject->GetDynamicParameters().SupportsFloat64VertexAttributes) { return VK_FORMAT_UNDEFINED; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h index 7e13525e..b14231bb 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h @@ -23,6 +23,9 @@ namespace MobileGL::MG_Backend::DirectVulkan { None = 0, Repack, ScaledIntegerToFloat32, + // GL_DOUBLE source data narrowed to a tightly packed float32 stream: the fetch half + // of the fp64 demotion the shader side already does unconditionally. + Float64ToFloat32, }; struct BackendVertexInputState { diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 2a1b0541..92faee42 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -972,6 +972,36 @@ namespace MobileGL::MG_Backend::DirectVulkan { } } + // The fetch half of the fp64 demotion the shader side already does unconditionally + // (DemoteFloat64Pass): the source bytes are ordinary IEEE-754 doubles, so a GL_DOUBLE array is + // deinterleaved into a tightly packed float32 stream rather than dropped. `normalized` is not + // consulted - GL ignores it for floating-point array types. + static Bool ConvertFloat64VertexStreamToFloat32( + const MG_State::GLState::VertexAttribute& attribute, + const Uint8* sourceData, + SizeT sourceStride, + SizeT elementCount, + Vector& outData) { + if (sourceData == nullptr || attribute.Size < 1 || attribute.Size > 4 || sourceStride == 0) { + return false; + } + + const SizeT componentCount = static_cast(attribute.Size); + outData.resize(elementCount * componentCount); + for (SizeT element = 0; element < elementCount; ++element) { + const Uint8* sourceElement = sourceData + element * sourceStride; + Float* destinationElement = outData.data() + element * componentCount; + for (SizeT component = 0; component < componentCount; ++component) { + // GL byte strides and offsets are arbitrary, so no component carries an 8-byte + // alignment guarantee; copy it out before narrowing it. + Double value = 0.0; + Memcpy(&value, sourceElement + component * sizeof(Double), sizeof(Double)); + destinationElement[component] = static_cast(value); + } + } + return true; + } + static Bool RepackVertexStream(const Uint8* sourceData, SizeT sourceStride, SizeT elementSize, @@ -3595,6 +3625,14 @@ void main() { uploadData = m_vertexConversionScratch.data(); uploadSize = static_cast(m_vertexConversionScratch.size() * sizeof(Float)); break; + case VertexInputStateFactory::VertexStreamConversion::Float64ToFloat32: + if (!ConvertFloat64VertexStreamToFloat32(attribute, sourceData, sourceStride, elementCount, + m_vertexConversionScratch)) { + return false; + } + uploadData = m_vertexConversionScratch.data(); + uploadSize = static_cast(m_vertexConversionScratch.size() * sizeof(Float)); + break; case VertexInputStateFactory::VertexStreamConversion::None: return false; } @@ -8926,6 +8964,7 @@ void main() { outMapping.baseSlice = baseSlice; outMapping.availableSlices = std::max(1u, image.depth >> mipLevel); return true; + case TextureTarget::Texture1DArray: case TextureTarget::Texture2DArray: case TextureTarget::Texture2DMultisampleArray: case TextureTarget::TextureCubeMap: @@ -8933,15 +8972,18 @@ void main() { // A cube map is an array of six faces here (see TryResolveTextureShapeInfo), and GL // numbers its faces on the same z axis an array texture numbers its layers, so both // arrive as a plain layer range. + // + // GL_TEXTURE_1D_ARRAY belongs here too, and needs no remap: this backend STORES it + // as a VK_IMAGE_TYPE_1D image whose layers live in arrayLayers (ToVulkanLevelExtent + // moves the count across), and GL 4.6 core 18.3.2 ADDRESSES it as a stack of slices + // on z with an image height of 1 - so the frontend's y/height are already the 0/1 + // Vulkan requires and the layer lands in baseArrayLayer either way. outMapping.slicesAreDepth = false; outMapping.baseSlice = baseSlice; outMapping.availableSlices = image.arrayLayers; return true; default: - // GL_TEXTURE_1D_ARRAY carries its layers on the Y axis (srcY/srcHeight), which - // would have to be remapped against a Vulkan extent that also has to stay height 1 - // for a VK_IMAGE_TYPE_1D image; GL_TEXTURE_BUFFER has no image at all. Declined - // rather than mis-addressed. + // GL_TEXTURE_BUFFER has no image at all. Declined rather than mis-addressed. return false; } } diff --git a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp index ceaf42fb..fe5cc80b 100644 --- a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp +++ b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp @@ -199,11 +199,37 @@ namespace MobileGL::MG_Impl::GLImpl { return false; } + const auto& currentProgram = MG_State::pGLContext->GetProgramForDraw(); + + // GL 4.6 core 10.1: the tessellation pipeline's only input primitive is GL_PATCHES, and + // GL_PATCHES has no meaning without it. Both directions are INVALID_OPERATION, and + // neither was implemented - which is two of the four sites + // KHR-GL43.transform_feedback.api_errors_test checks with one shared message string. + // The EVALUATION stage is what decides: a control stage cannot run without one, and a + // program carrying only an evaluation stage still tessellates, through GL's + // fixed-function pass-through control stage (11.2.2). + const Bool tessellationActive = + currentProgram && currentProgram->GetShaderIndexByStage(ShaderStage::TessEval) >= 0; + if (tessellationActive && mode != GL_PATCHES) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique( + "MG_Impl/GLImpl", functionName, + "A program with a tessellation evaluation shader can only be drawn with GL_PATCHES.")); + return false; + } + if (!tessellationActive && mode == GL_PATCHES) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", functionName, + "GL_PATCHES requires an active tessellation evaluation shader.")); + return false; + } + // A geometry stage only accepts the primitive types that decompose into its declared // input primitive (GL 4.6 core 11.3.1); anything else is INVALID_OPERATION. GL_PATCHES // is the tessellation pipeline's input and reaches the geometry stage already // converted, so it is not constrained here. - const auto& currentProgram = MG_State::pGLContext->GetProgramForDraw(); const GLenum gsInput = currentProgram ? currentProgram->GetGeometryInputType() : GL_NONE; if (gsInput != GL_NONE && mode != GL_PATCHES) { Bool compatible = false; @@ -239,13 +265,17 @@ namespace MobileGL::MG_Impl::GLImpl { // While transform feedback is active the draw's primitive type must match // the feedback primitive mode (GL 3.3 core 13.2.2). With a geometry shader // the constraint moves to the shader's output primitive type instead, so - // the draw mode itself is unconstrained here. A paused span is exempt: it - // captures nothing, so there is nothing for the mode to be incompatible with - // (GL 4.6 core 13.2.3). + // the draw mode itself is unconstrained here - and a TESSELLATION EVALUATION + // stage relocates it exactly the same way (GL 4.6 core 13.2.2 names both): + // what is captured is the tessellator's output primitive, and the draw mode + // can only ever be GL_PATCHES. A paused span is exempt: it captures nothing, + // so there is nothing for the mode to be incompatible with (GL 4.6 core 13.2.3). + const auto& feedbackProgram = MG_State::pGLContext->GetTransformFeedbackProgram(); + const Bool feedbackModeIsProgramDriven = + feedbackProgram && (feedbackProgram->GetShaderIndexByStage(ShaderStage::Geometry) >= 0 || + feedbackProgram->GetShaderIndexByStage(ShaderStage::TessEval) >= 0); if (MG_State::pGLContext->IsTransformFeedbackActive() && - !MG_State::pGLContext->IsTransformFeedbackPaused() && - !(MG_State::pGLContext->GetTransformFeedbackProgram() && - MG_State::pGLContext->GetTransformFeedbackProgram()->GetShaderIndexByStage(ShaderStage::Geometry) >= 0)) { + !MG_State::pGLContext->IsTransformFeedbackPaused() && !feedbackModeIsProgramDriven) { const GLenum feedbackMode = MG_State::pGLContext->GetTransformFeedbackPrimitiveMode(); Bool compatible = false; switch (feedbackMode) { diff --git a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp index ce1284ea..fef1fc52 100644 --- a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp +++ b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp @@ -528,6 +528,10 @@ namespace MobileGL::MG_Impl::GLImpl { case GL_UNIFORM_ARRAY_STRIDE: case GL_UNIFORM_MATRIX_STRIDE: case GL_UNIFORM_IS_ROW_MAJOR: + // GL 4.2 / ARB_shader_atomic_counters adds this one to the accepted set. Leaving it + // out did not merely lose the answer: the leftover GL_INVALID_ENUM is what made + // KHR-GL43.shader_atomic_counters.basic-program-query force a FAIL. + case GL_UNIFORM_ATOMIC_COUNTER_BUFFER_INDEX: break; default: MG_State::pGLContext->RecordError( @@ -583,6 +587,11 @@ namespace MobileGL::MG_Impl::GLImpl { case GL_UNIFORM_IS_ROW_MAJOR: params[i] = programObject->GetActiveUniformIsRowMajor(idx); break; + case GL_UNIFORM_ATOMIC_COUNTER_BUFFER_INDEX: + // Index into the GL_ACTIVE_ATOMIC_COUNTER_BUFFERS list, -1 for every uniform + // that is not an atomic counter (GL 4.6 core table 7.6). + params[i] = programObject->GetActiveUniformAtomicCounterBufferIndex(idx); + break; default: break; } diff --git a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp index 4ad78d9b..f0077c71 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp +++ b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp @@ -3750,19 +3750,34 @@ namespace MobileGL::MG_Impl::GLImpl { return GetCopyImageLevelSize(endpoint.Texture, uploadTarget, level); } - // How far the region's z axis may reach. It does not mean the same thing on every target - // GL 4.6 core 18.3.2 accepts: on a CUBE MAP it selects among the six faces, which this - // frontend keeps as six separate one-slice upload targets - so the level's own extent - // says 1 and the real bound is 6. A cube-map ARRAY is one upload target whose depth - // already counts layer-faces, and a 1D array carries its layers on y (which is where GL - // puts them for this entry point too), so both are answered by the level extent. - Int GetCopyImageEndpointLayerCount(const MG_Backend::CopyImageEndpoint& endpoint, - const IntVec3& levelSize) { - if (!endpoint.IsRenderbuffer() && endpoint.Texture && - endpoint.Texture->GetTarget() == TextureTarget::TextureCubeMap) { - return 6; + // The per-axis extent of one endpoint's image AS THIS ENTRY POINT ADDRESSES IT, which is + // not always the level extent this frontend stores. + // + // GL 4.6 core 18.3.2 treats EVERY array texture as a stack of slices addressed by z, and + // gives a 1D array an image height of 1. This frontend stores a 1D array the way + // glTexImage2D(GL_TEXTURE_1D_ARRAY, w, layers) writes it instead - layers on y - so the + // two views have to be told apart here. Measuring y against the LAYER count is what let + // srcY = 14 on a 16-wide, 16-layer 1D array come back GL_NO_ERROR + // (KHR-GL43.copy_image.exceeding_boundaries, the src_test_case y variants); the CTS is + // unambiguous about the convention, forcing height = 1 for 1D and 1D_ARRAY and listing + // 1D_ARRAY as multilayer. + // + // A CUBE MAP is the other target whose z bound is not the level extent: this frontend + // keeps its six faces as six separate one-slice upload targets, so the level says 1 and + // the real bound is 6. A cube-map ARRAY is one upload target whose depth already counts + // layer-faces, and every remaining target is answered by the level extent verbatim. + IntVec3 GetCopyImageEndpointRegionBounds(const MG_Backend::CopyImageEndpoint& endpoint, + const IntVec3& levelSize) { + const TextureTarget target = (!endpoint.IsRenderbuffer() && endpoint.Texture) + ? endpoint.Texture->GetTarget() + : TextureTarget::Unknown; + if (target == TextureTarget::TextureCubeMap) { + return {levelSize.x(), levelSize.y(), 6}; } - return std::max(levelSize.z(), 1); + if (target == TextureTarget::Texture1DArray) { + return {levelSize.x(), 1, std::max(levelSize.y(), 1)}; + } + return {levelSize.x(), levelSize.y(), std::max(levelSize.z(), 1)}; } // GL 4.6 core 18.3.2 requires INVALID_VALUE when the region exceeds either image's @@ -3780,9 +3795,9 @@ namespace MobileGL::MG_Impl::GLImpl { // reject a copy GL allows. Every caller has already established that the level // exists and that the image is complete, so this is a belt-and-braces guard. if (levelSize.x() <= 0 || levelSize.y() <= 0) return true; - const Int layers = GetCopyImageEndpointLayerCount(endpoint, levelSize); - if (x >= 0 && y >= 0 && z >= 0 && static_cast(x) + width <= levelSize.x() && - static_cast(y) + height <= levelSize.y() && static_cast(z) + depth <= layers) { + const IntVec3 bounds = GetCopyImageEndpointRegionBounds(endpoint, levelSize); + if (x >= 0 && y >= 0 && z >= 0 && static_cast(x) + width <= bounds.x() && + static_cast(y) + height <= bounds.y() && static_cast(z) + depth <= bounds.z()) { return true; } MG_State::pGLContext->RecordError( @@ -3791,7 +3806,7 @@ namespace MobileGL::MG_Impl::GLImpl { "MG_Impl/GLImpl", "ValidateCopyImageSubData_State", std::format("The {} region [{}, {}, {}] + [{} x {} x {}] does not fit inside the {} x {} x {} " "image.", - endpointName, x, y, z, width, height, depth, levelSize.x(), levelSize.y(), layers))); + endpointName, x, y, z, width, height, depth, bounds.x(), bounds.y(), bounds.z()))); return false; } } // namespace diff --git a/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp b/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp index cfffb533..29b07372 100644 --- a/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp +++ b/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp @@ -514,15 +514,17 @@ namespace MobileGL::MG_Impl::GLImpl { // recorded DataType is always Float64 - what IsLong adds is that this is the *unconverted* form, // as opposed to VertexAttribFormat(GL_DOUBLE), which asks for a float conversion. // - // Whether the backend can FEED it is detected, not assumed: DirectVulkan needs shaderFloat64, - // and DirectGLES can never have it at all. What that costs is the ARRAY, not the call: GL 4.6 - // core 10.3.2 defines no error for a well-formed glVertexAttribLFormat, and a GL 4.3 context - // has 64-bit attributes in core, so declining the call would be non-conformant and would make - // the four pure state queries (VERTEX_ATTRIB_ARRAY_SIZE / _TYPE / _LONG / _RELATIVE_OFFSET) - // unanswerable (KHR-GL43.vertex_attrib_binding.basic-state1/3). The format is therefore - // RECORDED here and the enabled array is dropped at draw instead - loudly, once, naming the - // reason. The matching startup POST row is in MG_Util/SelfTest/DriverPost.cpp; the draw-side - // drop is DirectGLES/Managers.cpp and, on DirectVulkan, VertexInputStateFactory's Float64 case. + // Whether the backend can FEED it at full precision is detected, not assumed: DirectVulkan + // needs shaderFloat64, and DirectGLES can never have it at all. What that costs is PRECISION, + // not the call and no longer the array: GL 4.6 core 10.3.2 defines no error for a well-formed + // glVertexAttribLFormat, and a GL 4.3 context has 64-bit attributes in core, so declining the + // call would be non-conformant and would make the four pure state queries + // (VERTEX_ATTRIB_ARRAY_SIZE / _TYPE / _LONG / _RELATIVE_OFFSET) unanswerable + // (KHR-GL43.vertex_attrib_binding.basic-state1/3). The format is therefore RECORDED here and + // the array is NARROWED to float32 at draw, matching the fp64 demotion every shader already + // gets (DemoteFloat64Pass) - loudly, once, naming the cost. The matching startup POST row is in + // MG_Util/SelfTest/DriverPost.cpp; the draw-side narrowing is DirectGLES/Managers.cpp and, on + // DirectVulkan, VertexInputStateFactory's Float64 case. static void VertexAttribLFormatSeparate_State(const SharedPtr& vao, GLuint attribindex, GLint size, GLenum type, GLuint relativeoffset) { @@ -534,9 +536,9 @@ namespace MobileGL::MG_Impl::GLImpl { !MG_Backend::pActiveBackendObject->GetDynamicParameters().SupportsFloat64VertexAttributes) { MGLOG_W_ONCE("VertexAttribLFormat: attribute %u asked for a 64-bit (GL_DOUBLE) format, but this " "backend has no double-precision vertex attribute support - the format is recorded " - "and queryable, but the array will be DROPPED at draw and the attribute will read " - "its generic current value; see the \"64-bit vertex attributes\" / \"shaderFloat64\" " - "POST row for what that costs", + "and queryable, and the array is FETCHED AT FLOAT32 PRECISION at draw (the same " + "narrowing the shader's dvec inputs already get); see the \"64-bit vertex " + "attributes\" / \"shaderFloat64\" POST row for what that costs", attribindex); } diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index fc8d89c2..e14dc712 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -85,6 +85,7 @@ add_executable(MobileGLIntegrationTest Scenarios/SsboDeclarationFormScenario.cpp Scenarios/Glsl420DeclarationScenario.cpp Scenarios/IoBlockNameCollisionScenario.cpp + Scenarios/TessellationDrawModeScenario.cpp Scenarios/FragmentOutputArrayIndexScenario.cpp Scenarios/BufferTextureScenario.cpp Scenarios/VertexAttribBindingScenario.cpp @@ -97,6 +98,8 @@ add_executable(MobileGLIntegrationTest Scenarios/LayeredAttachmentBarrierScenario.cpp Scenarios/LayeredTextureReadbackScenario.cpp Scenarios/AtomicCounterScenario.cpp + Scenarios/SsboArrayDynamicIndexScenario.cpp + Scenarios/StorageBufferRegrowScenario.cpp ) target_include_directories(MobileGLIntegrationTest PRIVATE diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp index df4fdded..abe912ee 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp @@ -66,6 +66,9 @@ namespace MGITest { constexpr int kExtent = 6; constexpr GLuint kFilledValue = 7u; constexpr GLuint kStoredValue = 13u; + // What the atomic cases add to a filled texel. Distinct from both values above, so a + // wrong answer cannot be read as either the untouched fill or a plain store. + constexpr GLuint kAtomicAddend = 5u; // Everything that differs between the eleven kinds, in one row. struct TargetKind { @@ -129,6 +132,25 @@ namespace MGITest { kind.imageType + " i0;\n\nvoid main()\n{\n " + StoreStatement(kind, "i0", "13u") + "\n}\n"; } + // The third direction, and the one neither of the two above can stand in for: an + // imageAtomic* reaches its texel through a SPIR-V operand path of its own + // (OpImageTexelPointer), not through OpImageRead or OpImageWrite. SPIRV-Cross's "ES has + // no 1D image, address it as 2D" coordinate widening is applied on the read and write + // paths and NOT on that one, so a 1D image whose loads and stores are both correct could + // still lose its entire stage to a single imageAtomicAdd - which is what + // KHR-GL4x.shader_image_load_store.basic-allTargets-atomic measured, with the driver + // answering "'imageAtomicAdd' : no matching overloaded function found". + // + // No readonly/writeonly here: an atomic needs both directions, and r32ui is one of the + // three formats GLSL ES exempts from the qualifier rule, so the bare declaration is legal. + // Returns the value the texel held BEFORE the add, so one dispatch checks the atomic's + // return value and the load case that follows checks its memory effect. + std::string SingleAtomicSource(const TargetKind& kind) { + return std::string(kComputePrologue) + "layout (location = 0, r32ui) coherent uniform " + + kind.imageType + " i0;\n" + kResultBlock + "void main()\n{\n ssb.sum = imageAtomicAdd(i0, " + + kind.coord + (kind.multisample ? ", 0, " : ", ") + std::to_string(kAtomicAddend) + "u);\n}\n"; + } + class ImageTargetKindScenario : public ScenarioTest { protected: void TearDown() override { @@ -374,6 +396,39 @@ namespace MGITest { glUseProgram(0); } + // Fill a texture of `kind`, add to texel (0,0,0) atomically, and require BOTH the + // value the atomic returned and the value it left behind. The read-back runs as a + // second program, for the same reason the store case does: a backend that gets the + // atomic's return right and its memory effect wrong cannot cancel itself out. + void RunAtomicCase(const TargetKind& kind) { + const GLuint atomicProgram = MakeComputeProgram(SingleAtomicSource(kind)); + const GLuint loadProgram = MakeComputeProgram(SingleLoadSource(kind)); + if (atomicProgram == 0 || loadProgram == 0) return; + const GLuint texture = MakeTexture(kind, true); + if (texture == 0) return; + const GLuint ssbo = MakeResultBuffer(); + + glBindImageTexture(0, texture, 0, GL_TRUE, 0, GL_READ_WRITE, GL_R32UI); + ASSERT_EQ(FirstGLError(), 0u) << kind.name << ": glBindImageTexture errored"; + + glUseProgram(atomicProgram); + glUniform1i(0, 0); + glDispatchCompute(1, 1, 1); + glMemoryBarrier(GL_ALL_BARRIER_BITS); + EXPECT_EQ(FirstGLError(), 0u) << kind.name << ": the atomic dispatch leaked a GL error"; + EXPECT_EQ(ReadResult(ssbo), kFilledValue) + << kind.name << ": imageAtomicAdd did not return the value the texel held before it"; + + glUseProgram(loadProgram); + glUniform1i(0, 0); + glDispatchCompute(1, 1, 1); + glMemoryBarrier(GL_ALL_BARRIER_BITS); + EXPECT_EQ(FirstGLError(), 0u) << kind.name << ": the loading dispatch leaked a GL error"; + EXPECT_EQ(ReadResult(ssbo), kFilledValue + kAtomicAddend) + << kind.name << ": imageAtomicAdd did not leave the sum in the texel"; + glUseProgram(0); + } + // The same texture, bound four times over, varying nothing but `layered` and `layer`. // // GL 4.6 core 8.26 (and ES 3.2 8.22, word for word): "If the texture identified by @@ -515,6 +570,29 @@ namespace MGITest { #undef MGL_DEFINE_LOAD_CASE #undef MGL_DEFINE_STORE_CASE + // ---- and the atomic direction, on the two kinds ES has to emulate ------- + // + // Deliberately NOT every kind. imageAtomic* takes its own SPIR-V operand path + // (OpImageTexelPointer), and the only kinds whose coordinate that path has to RESHAPE are the + // two 1D ones - everything else addresses its ES texture with the coordinate the application + // wrote. GL_TEXTURE_1D_ARRAY is the control (its reshape has been in + // Lower1DArrayImagesForEssl from the start, and basic-allTargets-atomic passes on it); + // GL_TEXTURE_1D is the one that had none, so `imageAtomicAdd(g_image_1d, coord.x, 2)` reached + // the driver as a scalar against an iimage2D and took the whole fragment stage - and its six + // other images - with it. + +#define MGL_DEFINE_ATOMIC_CASE(CaseName, Kind) \ + TEST_F(ImageTargetKindScenario, AtomicallyAddsTo##CaseName) { \ + if (!Ready()) return; \ + if (!ImagesAreUsable()) GTEST_SKIP() << "no compute image uniforms"; \ + RunAtomicCase(Kind); \ + } + + MGL_DEFINE_ATOMIC_CASE(Texture1D, kKind1D) + MGL_DEFINE_ATOMIC_CASE(Texture1DArray, kKind1DArray) + +#undef MGL_DEFINE_ATOMIC_CASE + // ---- and the same texture bound four times, varying only layered/layer --- // // KHR-GL42.bind_image_texture.single_layer's sweep, on the kinds whose backend target has diff --git a/MobileGL/MG_IntegrationTest/Scenarios/SsboArrayDynamicIndexScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/SsboArrayDynamicIndexScenario.cpp new file mode 100644 index 00000000..cb7948b4 --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/SsboArrayDynamicIndexScenario.cpp @@ -0,0 +1,189 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/SsboArrayDynamicIndexScenario.cpp +// Copyright (c) 2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// Scenario - A NON-CONSTANT INDEX INTO AN ARRAY OF SHADER STORAGE BLOCKS. +// +// GL 4.3 allows any dynamically-uniform expression there; GLSL ES keeps the ES 3.1 rule that the +// index must be a constant integral expression, and the Qualcomm compiler enforces it: +// +// '[' : indexing into an SSBO array using a non-constant expression is not permitted +// +// The stage then never compiles, the backend program links nothing, and every dispatch is a +// silent no-op - while glGetProgramiv(GL_LINK_STATUS) keeps reporting the successful link the +// frontend already published. That is why the conformance failures +// (KHR-GL43.shader_storage_buffer_object.basic-stdLayout-case1/case4, +// advanced-indirectAddressing-case2, compute_shader.resources-max, 7 cases in all) read back as +// "the buffer was never written" rather than as an error, and why this scenario asserts on +// contents rather than on link status. +// +// Both index shapes the legalization has to cover are exercised in one dispatch: a loop induction +// variable (which folds when the loop unrolls) and a `uniform int` (which nothing can fold, so the +// switch/select lowering is what carries it), for a read AND for a write. + +#include +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + // Bindings 0..3 are the block array, 4 is the output. + constexpr const char* kComputeSource = R"(#version 430 core +layout(local_size_x = 1) in; +layout(std430, binding = 0) buffer Slot { + uint value; +} g_slots[4]; +layout(std430, binding = 4) buffer Output { + uint g_result[]; +}; +uniform int g_index; +void main() { + // Loop-derived index: foldable by unrolling. + for (int i = 0; i < 4; ++i) { + g_result[i] = g_slots[i].value; + } + // Uniform-derived index: not foldable, read and write both. + g_result[4] = g_slots[g_index].value; + g_slots[g_index].value = 99u; +} +)"; + + constexpr int kSlotCount = 4; + constexpr int kResultCount = 5; + + class SsboArrayDynamicIndexScenario : public ScenarioTest { + protected: + void SetUp() override { + ScenarioTest::SetUp(); + if (!Ready()) return; + GLint blocks = 0; + glGetIntegerv(GL_MAX_COMPUTE_SHADER_STORAGE_BLOCKS, &blocks); + if (blocks < kSlotCount + 1) { + GTEST_SKIP() << "GL_MAX_COMPUTE_SHADER_STORAGE_BLOCKS is " << blocks << "; this needs " + << kSlotCount + 1; + } + m_program = CompileComputeProgram(kComputeSource); + ASSERT_NE(m_program, 0u) << m_buildLog; + } + + void TearDown() override { + if (!Ready()) return; + if (!m_buffers.empty()) glDeleteBuffers(static_cast(m_buffers.size()), m_buffers.data()); + if (m_program != 0) glDeleteProgram(m_program); + } + + unsigned int CompileComputeProgram(const char* source) { + const GLuint shader = glCreateShader(GL_COMPUTE_SHADER); + glShaderSource(shader, 1, &source, nullptr); + glCompileShader(shader); + GLint compiled = 0; + glGetShaderiv(shader, GL_COMPILE_STATUS, &compiled); + if (compiled == GL_FALSE) { + char log[2048] = {}; + glGetShaderInfoLog(shader, sizeof(log) - 1, nullptr, log); + m_buildLog = std::string("compute shader did not compile: ") + log; + glDeleteShader(shader); + return 0; + } + const GLuint program = glCreateProgram(); + glAttachShader(program, shader); + glLinkProgram(program); + glDeleteShader(shader); + GLint linked = 0; + glGetProgramiv(program, GL_LINK_STATUS, &linked); + if (linked == GL_FALSE) { + char log[2048] = {}; + glGetProgramInfoLog(program, sizeof(log) - 1, nullptr, log); + m_buildLog = std::string("compute program did not link: ") + log; + glDeleteProgram(program); + return 0; + } + return program; + } + + GLuint MakeStorageBuffer(const std::vector& contents) { + GLuint buffer = 0; + glGenBuffers(1, &buffer); + glBindBuffer(GL_SHADER_STORAGE_BUFFER, buffer); + glBufferData(GL_SHADER_STORAGE_BUFFER, + static_cast(contents.size() * sizeof(unsigned int)), contents.data(), + GL_DYNAMIC_COPY); + m_buffers.push_back(buffer); + return buffer; + } + + static std::vector ReadBuffer(GLuint buffer, int count) { + std::vector values(static_cast(count), 0xDEADBEEFu); + glBindBuffer(GL_SHADER_STORAGE_BUFFER, buffer); + glGetBufferSubData(GL_SHADER_STORAGE_BUFFER, 0, + static_cast(values.size() * sizeof(unsigned int)), values.data()); + return values; + } + + unsigned int m_program = 0; + std::string m_buildLog; + std::vector m_buffers; + }; + + } // namespace + + TEST_F(SsboArrayDynamicIndexScenario, ReadsAndWritesTheBlockTheIndexNames) { + if (!Ready() || IsSkipped()) return; + + GLuint slots[kSlotCount] = {}; + for (int i = 0; i < kSlotCount; ++i) { + slots[i] = MakeStorageBuffer({static_cast(10 + i)}); + glBindBufferBase(GL_SHADER_STORAGE_BUFFER, static_cast(i), slots[i]); + } + const GLuint output = MakeStorageBuffer(std::vector(kResultCount, 0u)); + glBindBufferBase(GL_SHADER_STORAGE_BUFFER, kSlotCount, output); + ASSERT_EQ(FirstGLError(), 0u); + + glUseProgram(m_program); + const GLint indexLocation = glGetUniformLocation(m_program, "g_index"); + ASSERT_NE(indexLocation, -1); + glUniform1i(indexLocation, 2); + glDispatchCompute(1, 1, 1); + glMemoryBarrier(GL_BUFFER_UPDATE_BARRIER_BIT); + EXPECT_EQ(FirstGLError(), 0u); + + const std::vector result = ReadBuffer(output, kResultCount); + for (int i = 0; i < kSlotCount; ++i) { + EXPECT_EQ(result[static_cast(i)], static_cast(10 + i)) + << "g_slots[" << i << "] read through the loop index came back as " + << result[static_cast(i)] + << "; 0 means the stage never compiled and the dispatch was a silent no-op"; + } + EXPECT_EQ(result[4], 12u) << "g_slots[g_index] with g_index = 2 read back as " << result[4]; + + const std::vector written = ReadBuffer(slots[2], 1); + EXPECT_EQ(written[0], 99u) << "the uniform-indexed WRITE landed as " << written[0] + << " instead of 99 in g_slots[2]"; + // The write must have gone to element 2 and nowhere else. + for (int i = 0; i < kSlotCount; ++i) { + if (i == 2) continue; + const std::vector untouched = ReadBuffer(slots[i], 1); + EXPECT_EQ(untouched[0], static_cast(10 + i)) + << "g_slots[" << i << "] was overwritten by a write that named element 2"; + } + + for (int i = 0; i <= kSlotCount; ++i) { + glBindBufferBase(GL_SHADER_STORAGE_BUFFER, static_cast(i), 0); + } + } +} // namespace MGITest diff --git a/MobileGL/MG_IntegrationTest/Scenarios/StorageBufferRegrowScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/StorageBufferRegrowScenario.cpp new file mode 100644 index 00000000..1824ebab --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/StorageBufferRegrowScenario.cpp @@ -0,0 +1,156 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/StorageBufferRegrowScenario.cpp +// Copyright (c) 2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// Scenario - glBufferData GROWS A BUFFER THAT IS ALREADY BOUND AT AN INDEXED POINT. +// +// GL says the indexed binding follows the buffer object, so after the store is re-specified the +// shader sees the NEW extent. DirectGLES shadows the indexed bindings so a redundant +// glBindBufferBase can be skipped, and nothing used to invalidate that shadow when the store was +// re-specified underneath it - so on a driver that resolves a whole-buffer indexed binding's +// extent at BIND time (Adreno does; Mali does not) the shader kept seeing the OLD, smaller range. +// Stores past it are dropped and loads return zero, which is exactly what +// KHR-GL43.compute_shader.dispatch-indirect reported: the first iteration's 6 elements correct and +// everything past byte 24 zero, after the same buffer was re-specified from 24 to 96 bytes. +// +// The assertion is deliberately on the WHOLE grown range, so a partial write names the byte the +// stale extent stopped at. + +#include +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + constexpr const char* kComputeSource = R"(#version 430 core +layout(local_size_x = 1) in; +layout(std430, binding = 0) buffer Output { + uint g_data[]; +}; +void main() { + g_data[gl_GlobalInvocationID.x] = gl_GlobalInvocationID.x + 1u; +} +)"; + + constexpr int kSmallElements = 6; // 24 bytes - the first iteration's size + constexpr int kLargeElements = 24; // 96 bytes - what the second iteration grows to + + class StorageBufferRegrowScenario : public ScenarioTest { + protected: + void SetUp() override { + ScenarioTest::SetUp(); + if (!Ready()) return; + m_program = CompileComputeProgram(kComputeSource); + ASSERT_NE(m_program, 0u) << m_buildLog; + glGenBuffers(1, &m_buffer); + } + + void TearDown() override { + if (!Ready()) return; + if (m_buffer != 0) glDeleteBuffers(1, &m_buffer); + if (m_program != 0) glDeleteProgram(m_program); + } + + unsigned int CompileComputeProgram(const char* source) { + const GLuint shader = glCreateShader(GL_COMPUTE_SHADER); + glShaderSource(shader, 1, &source, nullptr); + glCompileShader(shader); + GLint compiled = 0; + glGetShaderiv(shader, GL_COMPILE_STATUS, &compiled); + if (compiled == GL_FALSE) { + char log[2048] = {}; + glGetShaderInfoLog(shader, sizeof(log) - 1, nullptr, log); + m_buildLog = std::string("compute shader did not compile: ") + log; + glDeleteShader(shader); + return 0; + } + const GLuint program = glCreateProgram(); + glAttachShader(program, shader); + glLinkProgram(program); + glDeleteShader(shader); + GLint linked = 0; + glGetProgramiv(program, GL_LINK_STATUS, &linked); + if (linked == GL_FALSE) { + char log[2048] = {}; + glGetProgramInfoLog(program, sizeof(log) - 1, nullptr, log); + m_buildLog = std::string("compute program did not link: ") + log; + glDeleteProgram(program); + return 0; + } + return program; + } + + void RespecifyTo(int elements) { + const std::vector zeros(static_cast(elements), 0u); + glBindBuffer(GL_SHADER_STORAGE_BUFFER, m_buffer); + glBufferData(GL_SHADER_STORAGE_BUFFER, + static_cast(zeros.size() * sizeof(unsigned int)), zeros.data(), + GL_DYNAMIC_COPY); + } + + std::vector DispatchAndRead(int elements) { + glUseProgram(m_program); + glDispatchCompute(static_cast(elements), 1, 1); + glMemoryBarrier(GL_BUFFER_UPDATE_BARRIER_BIT); + std::vector values(static_cast(elements), 0xDEADBEEFu); + glBindBuffer(GL_SHADER_STORAGE_BUFFER, m_buffer); + glGetBufferSubData(GL_SHADER_STORAGE_BUFFER, 0, + static_cast(values.size() * sizeof(unsigned int)), values.data()); + return values; + } + + unsigned int m_program = 0; + GLuint m_buffer = 0; + std::string m_buildLog; + }; + + } // namespace + + TEST_F(StorageBufferRegrowScenario, AGrownStoreIsVisibleThroughItsExistingIndexedBinding) { + if (!Ready() || IsSkipped()) return; + + // Iteration one: 24 bytes, bound once, six groups. + RespecifyTo(kSmallElements); + glBindBufferBase(GL_SHADER_STORAGE_BUFFER, 0, m_buffer); + ASSERT_EQ(FirstGLError(), 0u); + + const std::vector small = DispatchAndRead(kSmallElements); + ASSERT_EQ(FirstGLError(), 0u); + for (int i = 0; i < kSmallElements; ++i) { + ASSERT_EQ(small[static_cast(i)], static_cast(i + 1)) + << "the 24-byte iteration itself did not write element " << i; + } + + // Iteration two: the SAME buffer grows to 96 bytes with NO new glBindBufferBase, which is + // what the application is entitled to do and what the shadow used to swallow. + RespecifyTo(kLargeElements); + ASSERT_EQ(FirstGLError(), 0u); + + const std::vector large = DispatchAndRead(kLargeElements); + EXPECT_EQ(FirstGLError(), 0u); + for (int i = 0; i < kLargeElements; ++i) { + EXPECT_EQ(large[static_cast(i)], static_cast(i + 1)) + << "element " << i << " (byte " << i * 4 << ") of the grown store came back as " + << large[static_cast(i)] + << "; zero from element " << kSmallElements + << " on means the shader still saw the pre-growth extent"; + } + + glBindBufferBase(GL_SHADER_STORAGE_BUFFER, 0, 0); + } +} // namespace MGITest diff --git a/MobileGL/MG_IntegrationTest/Scenarios/TessellationDrawModeScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/TessellationDrawModeScenario.cpp new file mode 100644 index 00000000..c4c9489f --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/TessellationDrawModeScenario.cpp @@ -0,0 +1,226 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/TessellationDrawModeScenario.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// Scenario - GL_PATCHES AND THE TESSELLATION PIPELINE ARE EACH OTHER'S ONLY PARTNER. +// +// GL 4.6 core 10.1 states the rule in both directions, and both are GL_INVALID_OPERATION: +// a program with a tessellation evaluation shader may only be drawn with GL_PATCHES, and +// GL_PATCHES may only be drawn with such a program. MobileGL's draw-mode validator +// implemented the geometry-shader input-primitive rule and NOTHING for tessellation, which +// is two of the four sites KHR-GL43.transform_feedback.api_errors_test checks (all four +// share one copy-pasted message string, so the trace cannot say which one it stopped at). +// +// Needs a real context: the validator returns before either rule when no backend object is +// active, so the GPU-free negative-API suite cannot reach them. + +#include +#include +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + const char* const kVertexSource = R"(#version 420 core +void main() +{ + gl_Position = vec4(0.0, 0.0, 0.0, 1.0); +} +)"; + + const char* const kTessControlSource = R"(#version 420 core +layout(vertices = 1) out; +void main() +{ + gl_TessLevelOuter[0] = 1.0; + gl_TessLevelOuter[1] = 1.0; + gl_TessLevelOuter[2] = 1.0; + gl_TessLevelInner[0] = 1.0; + gl_out[gl_InvocationID].gl_Position = gl_in[0].gl_Position; +} +)"; + + const char* const kTessEvalSource = R"(#version 420 core +layout(triangles, equal_spacing, cw) in; +void main() +{ + gl_Position = gl_in[0].gl_Position; +} +)"; + + const char* const kFragmentSource = R"(#version 420 core +out vec4 fragColor; +void main() +{ + fragColor = vec4(0.0, 1.0, 0.0, 1.0); +} +)"; + + class TessellationDrawModeScenario : public ScenarioTest { + protected: + void SetUp() override { + ScenarioTest::SetUp(); + if (!Ready()) return; + glGenVertexArrays(1, &m_vao); + glBindVertexArray(m_vao); + if (!BackendHostsTessellation()) { + GTEST_SKIP() << "no tessellation stages on " << Gl().BackendName() << " (" + << Gl().RendererString() << "); there is no patch draw to validate"; + } + } + + void TearDown() override { + if (!Ready()) return; + glUseProgram(0); + for (const GLuint program : m_programs) { + glDeleteProgram(program); + } + m_programs.clear(); + glBindVertexArray(0); + if (m_vao != 0) glDeleteVertexArrays(1, &m_vao); + m_vao = 0; + } + + // The same real-backend probe IoBlockNameCollisionScenario uses: 0 on a DirectGLES + // driver without GL_EXT_tessellation_shader and on a DirectVulkan device without + // the tessellationShader feature. + static bool BackendHostsTessellation() { + GLint maxTessGenLevel = 0; + glGetIntegerv(GL_MAX_TESS_GEN_LEVEL, &maxTessGenLevel); + DrainErrors(); + return maxTessGenLevel >= 1; + } + + static void DrainErrors() { + for (int i = 0; i < 16 && glGetError() != GL_NO_ERROR; ++i) { + } + } + + GLuint BuildProgram(const std::vector>& stages) { + std::vector shaders; + bool ok = true; + for (const auto& [stage, source] : stages) { + const GLuint shader = glCreateShader(stage); + glShaderSource(shader, 1, &source, nullptr); + glCompileShader(shader); + GLint compiled = 0; + glGetShaderiv(shader, GL_COMPILE_STATUS, &compiled); + shaders.push_back(shader); + if (!compiled) { + m_buildLog = InfoLog(shader, true); + ok = false; + break; + } + } + if (!ok) { + for (const GLuint shader : shaders) glDeleteShader(shader); + return 0; + } + + const GLuint program = glCreateProgram(); + for (const GLuint shader : shaders) glAttachShader(program, shader); + glLinkProgram(program); + GLint linked = 0; + glGetProgramiv(program, GL_LINK_STATUS, &linked); + for (const GLuint shader : shaders) glDeleteShader(shader); + if (!linked) { + m_buildLog = InfoLog(program, false); + glDeleteProgram(program); + return 0; + } + m_programs.push_back(program); + return program; + } + + static std::string InfoLog(GLuint object, bool isShader) { + GLint length = 0; + if (isShader) { + glGetShaderiv(object, GL_INFO_LOG_LENGTH, &length); + } else { + glGetProgramiv(object, GL_INFO_LOG_LENGTH, &length); + } + std::vector buffer(static_cast(length) + 1, '\0'); + if (isShader) { + glGetShaderInfoLog(object, length + 1, nullptr, buffer.data()); + } else { + glGetProgramInfoLog(object, length + 1, nullptr, buffer.data()); + } + return buffer.data(); + } + + const std::string& BuildLog() const { return m_buildLog; } + + GLuint m_vao = 0; + std::vector m_programs; + std::string m_buildLog; + }; + + // A tessellation program drawn with anything but GL_PATCHES. + TEST_F(TessellationDrawModeScenario, TessellationProgramRejectsNonPatchModes) { + if (!Ready()) GTEST_SKIP(); + + const GLuint program = BuildProgram({{GL_VERTEX_SHADER, kVertexSource}, + {GL_TESS_CONTROL_SHADER, kTessControlSource}, + {GL_TESS_EVALUATION_SHADER, kTessEvalSource}, + {GL_FRAGMENT_SHADER, kFragmentSource}}); + ASSERT_NE(program, 0u) << "the tessellation program did not build: " << BuildLog(); + + glUseProgram(program); + glPatchParameteri(GL_PATCH_VERTICES, 1); + DrainErrors(); + + for (const GLenum mode : {static_cast(GL_POINTS), static_cast(GL_LINES), + static_cast(GL_TRIANGLES)}) { + glDrawArrays(mode, 0, 1); + EXPECT_EQ(glGetError(), static_cast(GL_INVALID_OPERATION)) + << "mode " << mode << " must not be accepted while tessellation is active"; + DrainErrors(); + } + + // The one mode that IS accepted still is - a rule keyed any wider would break every + // patch draw in the suite. + glDrawArrays(GL_PATCHES, 0, 1); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + DrainErrors(); + } + + // ... and the other direction: GL_PATCHES without a tessellation evaluation stage. + TEST_F(TessellationDrawModeScenario, PatchesRejectedWithoutATessellationEvaluationStage) { + if (!Ready()) GTEST_SKIP(); + + const GLuint program = + BuildProgram({{GL_VERTEX_SHADER, kVertexSource}, {GL_FRAGMENT_SHADER, kFragmentSource}}); + ASSERT_NE(program, 0u) << "the vertex/fragment program did not build: " << BuildLog(); + + glUseProgram(program); + glPatchParameteri(GL_PATCH_VERTICES, 1); + DrainErrors(); + + glDrawArrays(GL_PATCHES, 0, 1); + EXPECT_EQ(glGetError(), static_cast(GL_INVALID_OPERATION)) + << "GL_PATCHES has no meaning without a tessellation evaluation stage"; + DrainErrors(); + + // The same program with an ordinary mode is untouched. + glDrawArrays(GL_TRIANGLES, 0, 3); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + DrainErrors(); + } + + } // namespace +} // namespace MGITest diff --git a/MobileGL/MG_IntegrationTest/Scenarios/VertexAttribBindingScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/VertexAttribBindingScenario.cpp index ec8c5ada..04b3faf6 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/VertexAttribBindingScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/VertexAttribBindingScenario.cpp @@ -673,4 +673,86 @@ void main() { glDeleteProgram(program); } + // A GL_DOUBLE array is NARROWED to float32 and fetched, not dropped. No backend here has a + // 64-bit vertex format, but glVertexAttribFormat(GL_DOUBLE) is defined as "doubles in memory, + // converted to float" and the shader input is a plain vec4 either way, so nothing about fp64 + // is needed - only the fetch conversion (KHR-GL43.vertex_attrib_binding.basic-input-case4). + // Every value here is exact in float32, so the capture is an equality test. + TEST_F(VertexAttribBindingScenario, DoubleArrayIsFetchedAtFloat32Precision) { + if (!Ready()) GTEST_SKIP(); + ResetCurrentAttribs(); + + const double vertices[] = {100.0, 200.0, 300.0, 400.0}; + GLuint vbo = 0; + glGenBuffers(1, &vbo); + glBindBuffer(GL_ARRAY_BUFFER, vbo); + glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + glBindVertexBuffer(0, vbo, 0, 2 * static_cast(sizeof(double))); + glVertexAttribFormat(1, 2, GL_DOUBLE, GL_FALSE, 0); + glVertexAttribBinding(1, 0); + glEnableVertexAttribArray(1); + + const std::vector data = CapturePoints(m_program, m_xfbo, 2, 1); + EXPECT_TRUE(Vec4Is(data, 0, 1, 100.0f, 200.0f, 0.0f, 1.0f)); + EXPECT_TRUE(Vec4Is(data, 1, 1, 300.0f, 400.0f, 0.0f, 1.0f)); + + glDisableVertexAttribArray(1); + glDeleteBuffers(1, &vbo); + } + + // GL ignores `normalized` for floating-point array types, GL_DOUBLE included: the fetched + // values are the raw ones, not scaled into [0,1]. A conversion that forwarded the flag would + // return zeros here (KHR-GL43.vertex_attrib_binding.basic-input-case5). + TEST_F(VertexAttribBindingScenario, NormalizedIsIgnoredForDoubleArrays) { + if (!Ready()) GTEST_SKIP(); + ResetCurrentAttribs(); + + const double vertices[] = {0.0, 10.0, 20.0, 0.0}; + GLuint vbo = 0; + glGenBuffers(1, &vbo); + glBindBuffer(GL_ARRAY_BUFFER, vbo); + glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + glBindVertexBuffer(0, vbo, 0, 4 * static_cast(sizeof(double))); + glVertexAttribFormat(2, 4, GL_DOUBLE, GL_TRUE, 0); + glVertexAttribBinding(2, 0); + glEnableVertexAttribArray(2); + + const std::vector data = CapturePoints(m_program, m_xfbo, 1, 1); + EXPECT_TRUE(Vec4Is(data, 0, 2, 0.0f, 10.0f, 20.0f, 0.0f)); + + glDisableVertexAttribArray(2); + glDeleteBuffers(1, &vbo); + } + + // The LONG form asks for more precision than any backend here can give and gets the same + // float32 stream. IsLong must not gate the narrowing off + // (KHR-GL43.vertex_attrib_binding.advanced-bindingUpdate feeds its dvec3 this way). + TEST_F(VertexAttribBindingScenario, LongDoubleArrayIsFetchedAtFloat32Precision) { + if (!Ready()) GTEST_SKIP(); + ResetCurrentAttribs(); + + const double vertices[] = {1.0, 2.0, 3.0, 4.0, 5.0, 6.0}; + GLuint vbo = 0; + glGenBuffers(1, &vbo); + glBindBuffer(GL_ARRAY_BUFFER, vbo); + glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + glBindVertexBuffer(0, vbo, 0, 3 * static_cast(sizeof(double))); + glVertexAttribLFormat(3, 3, GL_DOUBLE, 0); + glVertexAttribBinding(3, 0); + glEnableVertexAttribArray(3); + + const std::vector data = CapturePoints(m_program, m_xfbo, 2, 1); + EXPECT_TRUE(Vec4Is(data, 0, 3, 1.0f, 2.0f, 3.0f, 1.0f)); + EXPECT_TRUE(Vec4Is(data, 1, 3, 4.0f, 5.0f, 6.0f, 1.0f)); + + glDisableVertexAttribArray(3); + glDeleteBuffers(1, &vbo); + } + } // namespace MGITest diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ViewportArrayScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ViewportArrayScenario.cpp index c53c3105..44a75679 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ViewportArrayScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ViewportArrayScenario.cpp @@ -520,5 +520,215 @@ void main() { fragColor = vec4(float(gsIndex) * 16.0 / 255.0, 0.0, 0.0, 1.0); } DestroyIntTarget(target); } + // --- 4. an explicitly EMPTY scissor box clips, it does not mean "never written" -------- + // + // Deliberately NOT a ViewportArrayScenario case, because it must run on DirectGLES - the + // backend that got it wrong - and that fixture skips there. It needs none of the routing: + // one viewport, one scissor rectangle, no geometry stage. + // + // glScissor(0, 0, 0, 0) is legal GL meaning "the scissor test rejects every fragment", + // but it is byte-identical to the all-zero rectangle a context starts with, whose meaning + // is the OPPOSITE ("the whole window", which the frontend cannot spell before a surface + // exists). DirectGLES resolved the collision from the EXTENT, so it substituted the whole + // surface for a deliberately empty box and inverted the request into "clip nothing" - + // and did so on every draw, at any origin, no matter how many times the application had + // already called glScissor. KHR-GL43.viewport_array.scissor_zero_dimension is the + // conformance shape of exactly this, and it is what the written-flag now separates. + + const char* const kFullScreenVertexSource = R"(#version 330 core +void main() { + // One clip-space-covering triangle straight from gl_VertexID: no buffers, no attributes, + // and nothing that could clip the draw except the scissor rectangle under test. + const vec2 corners[3] = vec2[3](vec2(-1.0, -1.0), vec2(3.0, -1.0), vec2(-1.0, 3.0)); + gl_Position = vec4(corners[gl_VertexID], 0.0, 1.0); +} +)"; + + const char* const kConstantIntFragmentSource = R"(#version 330 core +layout(location = 0) out int fragColor; +void main() { fragColor = 7; } +)"; + constexpr GLint kPainted = 7; + + class EmptyScissorScenario : public ScenarioTest { + protected: + void SetUp() override { + ScenarioTest::SetUp(); + if (!Ready()) return; + + m_program = BuildQuadProgram(); + ASSERT_NE(m_program, 0u) << "full-screen program failed to build: " << m_buildLog; + glGenVertexArrays(1, &m_vao); + glBindVertexArray(m_vao); + + glGenTextures(1, &m_texture); + glBindTexture(GL_TEXTURE_2D, m_texture); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + glTexImage2D(GL_TEXTURE_2D, 0, GL_R32I, kSurfaceSide, kSurfaceSide, 0, GL_RED_INTEGER, GL_INT, + nullptr); + glGenFramebuffers(1, &m_fbo); + glBindFramebuffer(GL_FRAMEBUFFER, m_fbo); + glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, m_texture, 0); + ASSERT_EQ(glCheckFramebufferStatus(GL_FRAMEBUFFER), GL_FRAMEBUFFER_COMPLETE) + << "R32I is required to be colour-renderable; an incomplete target would make every " + "assertion below vacuous"; + + glViewport(0, 0, kSurfaceSide, kSurfaceSide); + glDisable(GL_DEPTH_TEST); + ResetScissorState(); + ASSERT_EQ(glGetError(), GL_NO_ERROR) << "setup left a GL error behind"; + } + + void TearDown() override { + if (!Ready() || IsSkipped()) return; + // The context is shared with every other scenario in the process, and a leftover + // 0x0 scissor box with the test enabled would silently blank whatever runs next. + ResetScissorState(); + glScissor(0, 0, kSurfaceSide, kSurfaceSide); + if (m_vao != 0) glDeleteVertexArrays(1, &m_vao); + if (m_program != 0) glDeleteProgram(m_program); + glBindFramebuffer(GL_FRAMEBUFFER, 0); + if (m_fbo != 0) glDeleteFramebuffers(1, &m_fbo); + if (m_texture != 0) glDeleteTextures(1, &m_texture); + while (glGetError() != GL_NO_ERROR) { + } + } + + static void ResetScissorState() { + for (int i = 0; i < kViewportCount; ++i) { + glDisablei(GL_SCISSOR_TEST, static_cast(i)); + } + glDisable(GL_SCISSOR_TEST); + } + + // Uploaded, not cleared, for the reason FillIntTarget gives - and here for a second + // one that is decisive: glClear is ITSELF scissored, so a clear issued under the very + // state this case is testing would be clipped away and prove nothing. + void FillTarget() const { + const std::vector unwritten(static_cast(kSurfaceSide) * kSurfaceSide, kUnwritten); + glBindTexture(GL_TEXTURE_2D, m_texture); + glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, kSurfaceSide, kSurfaceSide, GL_RED_INTEGER, GL_INT, + unwritten.data()); + } + + static std::vector ReadTarget() { + std::vector pixels(static_cast(kSurfaceSide) * kSurfaceSide, 0); + glReadPixels(0, 0, kSurfaceSide, kSurfaceSide, GL_RED_INTEGER, GL_INT, pixels.data()); + return pixels; + } + + GLuint BuildQuadProgram() { + const GLuint vs = CompileOne(GL_VERTEX_SHADER, kFullScreenVertexSource); + if (vs == 0) return 0; + const GLuint fs = CompileOne(GL_FRAGMENT_SHADER, kConstantIntFragmentSource); + if (fs == 0) { + glDeleteShader(vs); + return 0; + } + const GLuint program = glCreateProgram(); + glAttachShader(program, vs); + glAttachShader(program, fs); + glLinkProgram(program); + GLint linked = 0; + glGetProgramiv(program, GL_LINK_STATUS, &linked); + glDeleteShader(vs); + glDeleteShader(fs); + if (linked) return program; + GLint length = 0; + glGetProgramiv(program, GL_INFO_LOG_LENGTH, &length); + std::vector log(static_cast(length > 1 ? length : 1), '\0'); + glGetProgramInfoLog(program, static_cast(log.size()), nullptr, log.data()); + m_buildLog = log.data(); + glDeleteProgram(program); + return 0; + } + + GLuint CompileOne(GLenum stage, const char* source) { + const GLuint shader = glCreateShader(stage); + glShaderSource(shader, 1, &source, nullptr); + glCompileShader(shader); + GLint compiled = 0; + glGetShaderiv(shader, GL_COMPILE_STATUS, &compiled); + if (compiled) return shader; + GLint length = 0; + glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &length); + std::vector log(static_cast(length > 1 ? length : 1), '\0'); + glGetShaderInfoLog(shader, static_cast(log.size()), nullptr, log.data()); + m_buildLog = log.data(); + glDeleteShader(shader); + return 0; + } + + std::string m_buildLog; + GLuint m_program = 0; + GLuint m_vao = 0; + GLuint m_fbo = 0; + GLuint m_texture = 0; + }; + + TEST_F(EmptyScissorScenario, AnExplicitlyEmptyScissorBoxClipsEveryFragment) { + // Positive control FIRST. Without it a regression that simply lost the draw entirely + // would sail through the half below, which only asserts that nothing was painted. + FillTarget(); + glEnable(GL_SCISSOR_TEST); + glScissor(0, 0, kSurfaceSide, kSurfaceSide); + glUseProgram(m_program); + glBindVertexArray(m_vao); + glDrawArrays(GL_TRIANGLES, 0, 3); + ASSERT_EQ(glGetError(), GL_NO_ERROR); + { + const std::vector pixels = ReadTarget(); + ASSERT_EQ(pixels.front(), kPainted) << "control: a full-surface scissor box must not clip"; + ASSERT_EQ(pixels.back(), kPainted) << "control: a full-surface scissor box must not clip"; + } + + // The case itself, and note it runs AFTER an explicit glScissor - the old + // extent-based sentinel misfired here too, which is what made this a live rendering + // bug and not just a first-frame startup quirk. + FillTarget(); + glScissor(0, 0, 0, 0); + glDrawArrays(GL_TRIANGLES, 0, 3); + ASSERT_EQ(glGetError(), GL_NO_ERROR); + { + const std::vector pixels = ReadTarget(); + for (size_t i = 0; i < pixels.size(); ++i) { + ASSERT_EQ(pixels[i], kUnwritten) + << "texel " << i << " was painted through a 0x0 scissor box: the empty rectangle was " + "substituted with the whole surface, inverting 'clip everything' into 'clip nothing'"; + } + } + } + + TEST_F(EmptyScissorScenario, IndexedZeroDimensionScissorBoxesClipEveryFragment) { + // The conformance shape: setup4x4Scissor(..., set_zeros=true) writes all 16 boxes + // through glScissorArrayv with zero extents at a 4x4 grid of origins and enables the + // test on every index. Index 0's box is (0, 0, 0, 0) - byte-identical to the + // never-written default - which is precisely the collision the written flag breaks. + // Backends that collapse every index to 0 (DirectGLES today) still pass: index 0's + // box is empty, so the draw is clipped away, which is what the case requires. + FillTarget(); + std::vector boxes(static_cast(kViewportCount) * 4, 0); + for (int i = 0; i < kViewportCount; ++i) { + boxes[static_cast(i) * 4 + 0] = (i % kGridSide) * kCellSize; + boxes[static_cast(i) * 4 + 1] = (i / kGridSide) * kCellSize; + // width and height stay 0 - that IS the case. + } + glScissorArrayv(0, kViewportCount, boxes.data()); + for (int i = 0; i < kViewportCount; ++i) { + glEnablei(GL_SCISSOR_TEST, static_cast(i)); + } + glUseProgram(m_program); + glBindVertexArray(m_vao); + glDrawArrays(GL_TRIANGLES, 0, 3); + ASSERT_EQ(glGetError(), GL_NO_ERROR); + + const std::vector pixels = ReadTarget(); + for (size_t i = 0; i < pixels.size(); ++i) { + ASSERT_EQ(pixels[i], kUnwritten) << "texel " << i << " was painted through a zero-extent indexed " + "scissor box"; + } + } + } // namespace } // namespace MGITest diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp index 9d7c2ebb..2fe4cf96 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp @@ -1313,6 +1313,24 @@ namespace MobileGL::MG_State::GLState { // span is left; grow the table instead of leaving the uniform without // a location (which would make it unsettable via glUniform*). const SizeT base = artifacts.uniformIndexInTProgram.size(); + // The growth stops at the pool GL advertises. GL 4.6 core 7.6.1 bounds every + // uniform location by GL_MAX_UNIFORM_LOCATIONS, and the conformance suite reads a + // returned location >= the advertised maximum as a failure outright + // (KHR-GLES31.explicit_uniform_location.uniform-loc-mix-with-implicit-max). Minting + // 4095, 4096, ... is strictly worse than refusing: those are locations no + // application may legally name and no later query can make legal, so they would + // only turn a link-time exhaustion into a silently unwritable uniform. Unreachable + // for any program that fits glslang's per-stage uniform-component limits - it takes + // a fragmented pool of thousands of explicitly-located slots to get here. + if (base + static_cast(locationSpan) > kMaxUniformLocations) { + artifacts.infoLog = std::format( + "Uniform locations exhausted: '{}' needs {} location(s) and no free span is left below " + "GL_MAX_UNIFORM_LOCATIONS ({}).", + uniform.name, locationSpan, kMaxUniformLocations); + DeferLog(std::format("ProgramObject {}: Link failed - {}", in.externalIndex, artifacts.infoLog)); + ProgramObject::ResetLinkArtifacts(artifacts); + return false; + } artifacts.uniformIndexInTProgram.resize(base + locationSpan, glslang::TQualifier::layoutLocationEnd); artifacts.uniformSamplerOrImageUnitIndex.resize(base + locationSpan, -1); diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h index 8a5d563f..341154bc 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h @@ -281,6 +281,9 @@ namespace MobileGL::MG_State::GLState { } GLenum GetActiveUniformType(Uint index) const { + // The lowered counter is a plain uint inside a synthesized block; what the GL + // client declared - and what glGetActiveUniform must report - is an atomic_uint. + if (IsActiveUniformAtomicCounter(index)) return GL_UNSIGNED_INT_ATOMIC_COUNTER; return UniformAt(TProgramUniformIndex(index)).glDefineType; } @@ -298,10 +301,57 @@ namespace MobileGL::MG_State::GLState { } Int GetActiveUniformBlockIndex(Uint index) const { + // An atomic counter is a DEFAULT-BLOCK uniform to GL, whatever block the + // transpiler lowered it onto (GL 4.6 core 7.6, table 7.6): -1. + if (IsActiveUniformAtomicCounter(index)) return -1; // Members of the synthesized global UBO are default-block uniforms to GL: -1. return GlBlockIndexFromTProgram(UniformAt(TProgramUniformIndex(index)).index); } + // The transpiler lowers every atomic_uint onto a synthesized gl_AtomicCounterBlock_N + // block, but GL keeps seeing an atomic counter as a default-block uniform of type + // GL_UNSIGNED_INT_ATOMIC_COUNTER that points at an atomic-counter BUFFER. These two + // answer for that GL-level declaration; without them the query surface reports the + // lowering instead (GL_UNSIGNED_INT, block index 0) and + // KHR-GL43.shader_atomic_counters.basic-program-query fails on both. + // + // The returned value is an index into the GL_ACTIVE_ATOMIC_COUNTER_BUFFERS list, i.e. + // the RANK of the owning counter block among the counter blocks in glslang's block + // order - exactly how ProgramInterface numbers the GL_ATOMIC_COUNTER_BUFFER + // resources glGetActiveAtomicCounterBufferiv answers from. -1 when this uniform is + // not an atomic counter. + // Answered from the OWNED reflection snapshot, never from Artifacts().program. This + // arrived reading the live TProgram, which is null for every program served from the + // translation cache's L1 - and unlike the other query-surface accessors that made the + // same mistake, this one DEREFERENCES it, so the second program built from a given set + // of sources would have taken the process down rather than answered wrongly. The + // snapshot carries the same three facts in the same TPROGRAM index space: + // getUniform(i).index -> UniformAt(i).index, getNumUniformBlocks() -> + // blockReflection.size(), getUniformBlock(i).name -> BlockAt(i).name. + Int GetActiveUniformAtomicCounterBufferIndex(Uint index) const { + const Int tIndex = TProgramUniformIndex(index); + if (tIndex < 0) return -1; + const Int owner = UniformAt(tIndex).index; + if (owner < 0) return -1; + const Int blockCount = static_cast(Artifacts().blockReflection.size()); + if (owner >= blockCount) return -1; + const SizeT prefixLength = StringView(MG_Util::ShaderTranspiler::ATOMIC_COUNTER_BLOCK_PREFIX).size(); + Int counterBufferIndex = 0; + for (Int i = 0; i < blockCount; ++i) { + const auto& blockName = BlockAt(i).name; + if (blockName.compare(0, prefixLength, MG_Util::ShaderTranspiler::ATOMIC_COUNTER_BLOCK_PREFIX) != 0) { + continue; + } + if (i == owner) return counterBufferIndex; + ++counterBufferIndex; + } + return -1; + } + + Bool IsActiveUniformAtomicCounter(Uint index) const { + return GetActiveUniformAtomicCounterBufferIndex(index) >= 0; + } + // GL_UNIFORM_OFFSET: byte offset within the owning named block; -1 for a default-block // uniform. The relaxed parse gives global-UBO members real byte offsets, but GL must keep // seeing them as default-block uniforms, so gate on the GL-visible block index. diff --git a/MobileGL/MG_State/GLState/ProgramState/ShaderCompileTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ShaderCompileTask.cpp index a421921c..e950b0ed 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ShaderCompileTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ShaderCompileTask.cpp @@ -195,6 +195,13 @@ namespace { return result; } + if (const std::optional counterOffsetError = + FindAtomicCounterOffsetViolation(result.preprocessedSource)) { + result.outcome = ShaderPreprocessOutcome::AtomicCounterOffsetRejected; + result.infoLog = *counterOffsetError; + return result; + } + // The parse this feeds runs in the link-compatible configuration (Vulkan-client // env with relaxed rules): the TShader it produces is what glLinkProgram links and // what the backends' SPIR-V is generated from - there is no second, GL-client diff --git a/MobileGL/MG_State/GLState/ProgramState/ShaderPreprocessCache.h b/MobileGL/MG_State/GLState/ProgramState/ShaderPreprocessCache.h index 238ff7ba..55ec4465 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ShaderPreprocessCache.h +++ b/MobileGL/MG_State/GLState/ProgramState/ShaderPreprocessCache.h @@ -29,6 +29,9 @@ namespace MobileGL::MG_State::GLState { // FindShaderStorageBindingViolation rejected it: a storage block declared a binding at or // past GL_MAX_SHADER_STORAGE_BUFFER_BINDINGS. ResourceBindingRejected, + // FindAtomicCounterOffsetViolation rejected it: an atomic counter declared a + // layout(offset =) that is misaligned or reaches past GL_MAX_ATOMIC_COUNTER_BUFFER_SIZE. + AtomicCounterOffsetRejected, // The source-only half was clean but glslang rejected the preprocessed source. // Memoizing this saves the parse itself on every later object with that source. ParseFailed, diff --git a/MobileGL/MG_State/GLState/RenderState/RenderState.cpp b/MobileGL/MG_State/GLState/RenderState/RenderState.cpp index f2eb4503..8a394c37 100644 --- a/MobileGL/MG_State/GLState/RenderState/RenderState.cpp +++ b/MobileGL/MG_State/GLState/RenderState/RenderState.cpp @@ -843,6 +843,21 @@ namespace MobileGL { stored = box; stateChanged = true; } + // "The application has written this rectangle" is a DIFFERENT predicate from "the + // value moved", and the backends need the first one: glScissor(0, 0, 0, 0) as the + // very first scissor call leaves every stored box byte-identical to its + // never-written default, and that call is precisely the one whose meaning a + // backend must stop guessing at (see ScissorBoxWrittenMask). + // + // The transition has to count as a state change for the version too. DirectGLES' + // SyncRenderState early-outs on an unchanged render-state version BEFORE it + // reaches the span memcmp that would otherwise notice the mask, so a version-less + // flag flip would sit in the parameter block and never be pushed. It is a + // once-per-index transition, so the steady state still costs nothing. + if (m_parameters.ScissorBoxWrittenMask != kAllViewportsMask) { + m_parameters.ScissorBoxWrittenMask = kAllViewportsMask; + stateChanged = true; + } if (stateChanged) ++m_version; } @@ -855,9 +870,15 @@ namespace MobileGL { MOBILEGL_ASSERT(false, "Scissor box index out of range: %u", index); return; } - if (m_parameters.ScissorBoxes[index] == box) return; + // See SetScissorBox: a first write is state even when it does not move the value, + // so the unchanged-value early-out may only fire once this index is already + // marked written. + const Uint32 writtenBit = 1u << index; + const Bool alreadyWritten = (m_parameters.ScissorBoxWrittenMask & writtenBit) != 0; + if (alreadyWritten && m_parameters.ScissorBoxes[index] == box) return; m_parameters.ScissorBoxes[index] = box; + m_parameters.ScissorBoxWrittenMask |= writtenBit; ++m_version; } diff --git a/MobileGL/MG_State/GLState/RenderState/RenderState.h b/MobileGL/MG_State/GLState/RenderState/RenderState.h index 73b51fbd..05e90b24 100644 --- a/MobileGL/MG_State/GLState/RenderState/RenderState.h +++ b/MobileGL/MG_State/GLState/RenderState/RenderState.h @@ -328,6 +328,18 @@ namespace MobileGL { // turns it into a real glEnable/glDisable. Uint32 ScissorTestEnabledMask = 0; Array ScissorBoxes{}; // x, y, width, height + // One bit per viewport, set the first time the application writes that index's scissor + // rectangle - glScissor broadcasts and sets all 16, glScissorIndexed/glScissorArrayv set + // the indices they name. It exists because the RECTANGLE cannot answer "has the + // application spoken?": ScissorBoxes starts all-zero (its spec initial value is the size + // of a window the frontend does not know yet, see the RenderState constructor), and + // glScissor(0, 0, 0, 0) is a legal GL state meaning "the scissor test rejects every + // fragment". A backend that reads an empty rectangle as the never-written sentinel + // therefore INVERTS that request into "accept every fragment"; DirectGLES did exactly + // that and KHR-GL43.viewport_array.scissor_zero_dimension caught it. Deliberately beside + // ScissorBoxes so it shares their tail span (after LogicOp) and DirectGLES' span memcmp + // picks a transition up like any other state. + Uint32 ScissorBoxWrittenMask = 0; // glEnable(GL_CLIP_DISTANCE0 + i) for i in [0, 8), one bit each. A bitmask rather than // eight bools because every consumer wants the set, not an individual flag, and because // the SYNC_CAPABILITY/SET_CAPABILITY macros key off a "Enabled" field name that diff --git a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp index 15d4b524..91ead160 100644 --- a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp +++ b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp @@ -233,6 +233,101 @@ void main() EXPECT_TRUE(Contains(out, "uniform writeonly highp image2D storeOnly;")) << out; } +// The ORDERING half of the split, which `coherent` alone does not buy. Coherent makes the store +// through one variable VISIBLE to a load through the other; it says nothing about the order of +// the two within a single invocation, and the ES compiler - seeing a write to one variable and a +// read of another it has no reason to believe alias - is free to serve the read from before the +// write. That is what advanced-memory-order measured on Adreno with the coherent pair already in +// place. memoryBarrierImage() is the primitive that orders them. +TEST(SplitReadWriteImageUniformsTest, EverySplitStoreIsFollowedByAnImageMemoryBarrier) { + const String source = R"(#version 320 es +layout(binding = 2, rgba8) uniform highp image2D goku; +layout(location = 0) out highp vec4 mg_FragColor; +void main() +{ + imageStore(goku, ivec2(0), vec4(1.0)); + highp vec4 first = imageLoad(goku, ivec2(0)); + imageStore(goku, ivec2(0), vec4(2.0)); + mg_FragColor = first + imageLoad(goku, ivec2(0)); +} +)"; + const String out = SplitReadWriteImageUniforms(source); + + EXPECT_TRUE(Contains(out, "imageStore(" + WriteAlias("goku") + ", ivec2(0), vec4(1.0)); memoryBarrierImage();")) + << out; + EXPECT_TRUE(Contains(out, "imageStore(" + WriteAlias("goku") + ", ivec2(0), vec4(2.0)); memoryBarrierImage();")) + << out; + // One per store, not one per shader and not one per load. + EXPECT_EQ(CountOf(out, "memoryBarrierImage();"), 2u) << out; +} + +// The barrier belongs to the SPLIT alone. A store-only image was repaired in place, nothing +// aliases it, and paying for a barrier there would slow down every shader that merely writes an +// image - which is most of them. +TEST(SplitReadWriteImageUniformsTest, ARepairedButUnsplitStoreGetsNoBarrier) { + const String source = R"(#version 320 es +layout(binding = 3, rgba8) uniform highp image2D storeOnly; +void main() +{ + imageStore(storeOnly, ivec2(0), vec4(1.0)); +} +)"; + const String out = SplitReadWriteImageUniforms(source); + EXPECT_TRUE(Contains(out, "uniform writeonly highp image2D storeOnly;")) << out; + EXPECT_FALSE(Contains(out, "memoryBarrierImage")) << out; +} + +// The store site is found by matching the call's own parentheses, not by looking for the next +// ')', so a nested call in the value argument does not truncate the statement and the barrier +// still lands after the whole thing. +TEST(SplitReadWriteImageUniformsTest, TheBarrierLandsAfterAStoreWithNestedParentheses) { + const String source = R"(#version 320 es +layout(binding = 6, rgba8) uniform highp image2D gohan[3]; +void main() +{ + imageStore(gohan[1], ivec2(0), max(imageLoad(gohan[2], ivec2(0)), vec4(0.5))); +} +)"; + const String out = SplitReadWriteImageUniforms(source); + EXPECT_TRUE(Contains(out, "max(imageLoad(gohan[2], ivec2(0)), vec4(0.5))); memoryBarrierImage();")) << out; + EXPECT_EQ(CountOf(out, "memoryBarrierImage();"), 1u) << out; +} + +// The split is the one thing that makes a stage declare MORE image uniforms than the application +// did, and MobileGL keeps advertising GL_MAX_*_IMAGE_UNIFORMS unadjusted (lowering it would fail +// basic-api and NotSupported-out every case that only uses readonly/writeonly images). So the +// count has to be reportable, or a link failure caused by the doubling looks like a driver +// mystery - which is what KHR-GL4x.shader_image_load_store.multiple-uniforms will hit the moment +// the format work stops masking it. +TEST(SplitReadWriteImageUniformsTest, TheSplitCountIsReportedToTheCaller) { + const String twoSplits = R"(#version 320 es +layout(binding = 0, rgba8) uniform highp image2D goku; +layout(binding = 1, rgba16f) uniform highp image2D gohan; +layout(binding = 2, rgba8) uniform highp image2D storeOnly; +void main() +{ + imageStore(goku, ivec2(0), imageLoad(goku, ivec2(0))); + imageStore(gohan, ivec2(0), imageLoad(gohan, ivec2(0))); + imageStore(storeOnly, ivec2(0), vec4(0.0)); +} +)"; + Uint splitCount = 99u; + SplitReadWriteImageUniforms(twoSplits, &splitCount); + EXPECT_EQ(splitCount, 2u) << "only the read+write pair counts; the store-only repair adds no uniform"; + + // Every early return has to write the count too, or a caller reads whatever was there before. + const String noImages = R"(#version 320 es +layout(location = 0) out highp vec4 mg_FragColor; +void main() +{ + mg_FragColor = vec4(1.0); +} +)"; + splitCount = 99u; + SplitReadWriteImageUniforms(noImages, &splitCount); + EXPECT_EQ(splitCount, 0u); +} + // imageSize reads no texels and writes none, so it decides nothing; readonly is what keeps // such a declaration legal. TEST(SplitReadWriteImageUniformsTest, ImageSizeAloneDoesNotCountAsALoadOrAStore) { diff --git a/MobileGL/MG_Test/Program/ProgramInterfaceTest.cpp b/MobileGL/MG_Test/Program/ProgramInterfaceTest.cpp index 16430327..8f6e1c86 100644 --- a/MobileGL/MG_Test/Program/ProgramInterfaceTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramInterfaceTest.cpp @@ -716,6 +716,86 @@ void main() { EXPECT_EQ(TakeError(), GL_INVALID_ENUM); } + // The CLASSIC query surface has to agree with the interface query above. MobileGL lowers + // every atomic_uint onto a synthesized gl_AtomicCounterBlock_N, and glGetActiveUniform / + // glGetActiveUniformsiv used to report that lowering: GL_UNSIGNED_INT instead of + // GL_UNSIGNED_INT_ATOMIC_COUNTER, the synthesized block's index instead of the -1 a + // default-block uniform owes, and GL_INVALID_ENUM for + // GL_UNIFORM_ATOMIC_COUNTER_BUFFER_INDEX - the last of which is what made + // KHR-GL43.shader_atomic_counters.basic-program-query a forced FAIL. + TEST_F(ProgramInterfaceTest, AtomicCounterClassicUniformQueries) { + const char* fs = R"(#version 430 +out vec4 color; +layout (binding = 0, offset = 0) uniform atomic_uint ac_counter0; +layout (binding = 1, offset = 0) uniform atomic_uint ac_counter1; +uniform float plain; +void main() { + color = vec4(float(atomicCounterIncrement(ac_counter0) + atomicCounterIncrement(ac_counter1)) + plain); +} +)"; + const GLuint p = MakeProgram(kSimpleVs, fs); + LinkProgram(p); + ExpectLinked(p); + ClearErrors(); + + const auto indexOf = [p](const char* name) { + const GLchar* names[1] = {name}; + GLuint index = GL_INVALID_INDEX; + GetUniformIndices(p, 1, names, &index); + return index; + }; + const auto uniformiv = [p](GLuint index, GLenum pname) { + GLint value = -12345; + const GLuint indices[1] = {index}; + GetActiveUniformsiv(p, 1, indices, pname, &value); + return value; + }; + + const GLuint counter0 = indexOf("ac_counter0"); + const GLuint counter1 = indexOf("ac_counter1"); + const GLuint plain = indexOf("plain"); + ASSERT_NE(counter0, GL_INVALID_INDEX); + ASSERT_NE(counter1, GL_INVALID_INDEX); + ASSERT_NE(plain, GL_INVALID_INDEX); + + // (a) glGetActiveUniform and glGetActiveUniformsiv(GL_UNIFORM_TYPE) both report the + // GL-level type. + GLint size = 0; + GLenum type = 0; + GLchar nameBuffer[64] = {'\0'}; + GetActiveUniform(p, counter0, sizeof(nameBuffer), nullptr, &size, &type, nameBuffer); + EXPECT_EQ(type, static_cast(GL_UNSIGNED_INT_ATOMIC_COUNTER)); + EXPECT_EQ(std::string(nameBuffer), "ac_counter0"); + EXPECT_EQ(uniformiv(counter0, GL_UNIFORM_TYPE), GL_UNSIGNED_INT_ATOMIC_COUNTER); + EXPECT_EQ(uniformiv(counter1, GL_UNIFORM_TYPE), GL_UNSIGNED_INT_ATOMIC_COUNTER); + EXPECT_EQ(uniformiv(plain, GL_UNIFORM_TYPE), GL_FLOAT); + + // (b) an atomic counter is a DEFAULT-BLOCK uniform, whatever it was lowered onto. + EXPECT_EQ(uniformiv(counter0, GL_UNIFORM_BLOCK_INDEX), -1); + EXPECT_EQ(uniformiv(counter1, GL_UNIFORM_BLOCK_INDEX), -1); + EXPECT_EQ(uniformiv(plain, GL_UNIFORM_BLOCK_INDEX), -1); + + // (c) the pname is accepted, answers with the buffer's index, and reports -1 for a + // uniform that is not a counter. Two bindings mean two distinct buffers. + const GLint buffer0 = uniformiv(counter0, GL_UNIFORM_ATOMIC_COUNTER_BUFFER_INDEX); + const GLint buffer1 = uniformiv(counter1, GL_UNIFORM_ATOMIC_COUNTER_BUFFER_INDEX); + EXPECT_GE(buffer0, 0); + EXPECT_GE(buffer1, 0); + EXPECT_NE(buffer0, buffer1); + EXPECT_LT(buffer0, Interfaceiv(p, GL_ATOMIC_COUNTER_BUFFER, GL_ACTIVE_RESOURCES)); + EXPECT_LT(buffer1, Interfaceiv(p, GL_ATOMIC_COUNTER_BUFFER, GL_ACTIVE_RESOURCES)); + EXPECT_EQ(uniformiv(plain, GL_UNIFORM_ATOMIC_COUNTER_BUFFER_INDEX), -1); + // No leftover error: the CTS harness fails the subcase on one. + EXPECT_EQ(TakeError(), GL_NO_ERROR); + + // The classic surface and the interface query name the same buffer. + const std::vector interfaceBuffer = + PropsOf(p, GL_UNIFORM, "ac_counter0", {GL_ATOMIC_COUNTER_BUFFER_INDEX}); + ASSERT_EQ(interfaceBuffer.size(), 1u); + EXPECT_EQ(interfaceBuffer[0], buffer0); + EXPECT_EQ(TakeError(), GL_NO_ERROR); + } + // Two counters that share a binding AND an offset must fail to link. glslang's own check // lives in fixOffset(), which the Vulkan-relaxed parse never reaches - it folds the // atomic_uint into a storage block and returns from declareVariable() first - so the pair diff --git a/MobileGL/MG_Test/Program/ProgramTest.cpp b/MobileGL/MG_Test/Program/ProgramTest.cpp index 92862471..0383837a 100644 --- a/MobileGL/MG_Test/Program/ProgramTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramTest.cpp @@ -1934,10 +1934,19 @@ TEST_F(ProgramTest, GetActiveUniformsivErrors) { EXPECT_EQ(GetError(), GL_INVALID_VALUE); EXPECT_EQ(params[0], -999); - // E3: GL 4.2 token -> GL_INVALID_ENUM here. + // E3: the GL 4.2 / ARB_shader_atomic_counters token is ACCEPTED, not rejected. + // + // This case used to assert GL_INVALID_ENUM, which was right only while the token was + // unimplemented. It is implemented now, and `validIndex` names an ordinary uniform rather + // than an atomic counter, so the spec answer is -1 with no error (GL 4.6 core table 7.6). + // ProgramInterfaceTest's atomic-counter case asserts the same -1 for a non-counter + // uniform; leaving this one inverted made the two contradict each other. GetActiveUniformsiv(program, 1, &validIndex, GL_UNIFORM_ATOMIC_COUNTER_BUFFER_INDEX, params); - EXPECT_EQ(GetError(), GL_INVALID_ENUM); - EXPECT_EQ(params[0], -999); + EXPECT_EQ(GetError(), GL_NO_ERROR); + EXPECT_EQ(params[0], -1); + // Restored: the cases below assert that a REJECTED call leaves params untouched, and this + // one legitimately wrote to it. + params[0] = -999; // E4a: a live shader name -> GL_INVALID_OPERATION. GLuint shader = CreateShader(GL_VERTEX_SHADER); @@ -2162,6 +2171,69 @@ void main() { EXPECT_EQ(GetError(), GL_NO_ERROR); } +// Repro for KHR-GLES31.explicit_uniform_location.uniform-loc-arrays-of-arrays: an +// array-of-arrays uniform reaches the GL surface as one entry PER SUB-ARRAY ("u0[0]", +// "u0[1]" - glslang stops expanding at reflection granularity), while SPIRV-Reflect keeps +// it as a single leaf carrying every dimension. Routing the single leaf only ever covered +// the first sub-array, so every element from u0[1][0] on found no UBO offset and fell +// through to the fallback scratch at the tail of the shadow - storage the GPU never reads, +// which made those glUniform writes silently vanish. +TEST_F(ProgramTest, ArrayOfArraysUniformElementOffsets) { + // Arrays of arrays need GLSL 4.30; both stages take the same version. + const char* vsSource = R"(#version 430 core +in vec4 a_position; +void main() { + gl_Position = a_position; +})"; + const char* fsSource = R"(#version 430 core +uniform float u0[2][3]; +uniform vec3 u1[2][2]; +out vec4 o_color; +void main() { + float s = 0.0; + for (int i = 0; i < 2; ++i) { + for (int j = 0; j < 3; ++j) s += u0[i][j]; + } + vec3 v = vec3(0.0); + for (int i = 0; i < 2; ++i) { + for (int j = 0; j < 2; ++j) v += u1[i][j]; + } + o_color = vec4(v, s); +})"; + GLuint program = LinkVsFsProgram(vsSource, fsSource); + UseProgram(program); + auto programObject = MG_State::pGLContext->GetProgramObject(program); + ASSERT_NE(programObject, nullptr); + + // std140 gives a float array element and a vec3 array element the same 16-byte slot, + // and a flattened array-of-arrays is one contiguous run of those slots. + constexpr Uint kStd140ElementStride = 16u; + + const auto checkFlattenedRun = [&](const char* base, int outer, int inner) { + Uint firstOffset = MG_State::GLState::ProgramObject::kInvalidUniformOffset; + for (int i = 0; i < outer; ++i) { + for (int j = 0; j < inner; ++j) { + const std::string name = + std::string(base) + "[" + std::to_string(i) + "][" + std::to_string(j) + "]"; + const GLint location = GetUniformLocation(program, name.c_str()); + ASSERT_GE(location, 0) << name; + const Uint offset = programObject->GetUniformOffset(static_cast(location)); + ASSERT_NE(offset, MG_State::GLState::ProgramObject::kInvalidUniformOffset) << name; + const Uint element = static_cast(i * inner + j); + if (element == 0) { + firstOffset = offset; + } else { + EXPECT_EQ(offset, firstOffset + element * kStd140ElementStride) << name; + } + } + } + }; + + checkFlattenedRun("u0", 2, 3); + checkFlattenedRun("u1", 2, 2); + EXPECT_EQ(GetError(), GL_NO_ERROR); +} + // --------------------------------------------------------------------------- // GL CTS KHR-GL33.shaders.uniform_block regression pack. MobileGL's SPIR-V // pipeline lays every uniform block out as std140; the frontend implements the diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 8c82509e..7478bce6 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -3348,11 +3348,38 @@ namespace { return count; } + // Same word walk, for the NON-arrayed half of the family (Arrayed == 0). + SizeT Count1DNonArrayedStorageImageTypes(const Vector& spirv) { + constexpr unsigned kOpTypeImage = 25, kDim1D = 0; + SizeT count = 0; + for (SizeT i = 5; i < spirv.size();) { + const unsigned wordCount = spirv[i] >> 16; + const unsigned opcode = spirv[i] & 0xFFFFu; + if (wordCount == 0 || i + wordCount > spirv.size()) break; + if (opcode == kOpTypeImage && wordCount >= 8 && spirv[i + 3] == kDim1D && spirv[i + 5] == 0u && + spirv[i + 7] == 2u) { + ++count; + } + i += wordCount; + } + return count; + } + const char* k1DArrayImageCompute = R"(#version 440 core layout (local_size_x = 1) in; layout (location = 0, r32ui) readonly uniform uimage1DArray i0; layout (std430, binding = 0) buffer SSB { uint sum; } ssb; void main() { ssb.sum = imageLoad(i0, ivec2(2, 3)).r; } +)"; + + // KHR-GL4x.shader_image_load_store.basic-allTargets-atomic's own shape, minus the six other + // targets: a non-arrayed 1D storage image reached ONLY through an atomic. r32ui because ES + // defines image atomics on r32i/r32ui/r32f alone. + const char* k1DImageAtomicCompute = R"(#version 440 core +layout (local_size_x = 1) in; +layout (r32ui) coherent uniform uimage1D i0; +layout (std430, binding = 0) buffer SSB { uint sum; } ssb; +void main() { ssb.sum = imageAtomicAdd(i0, 2, 7u); } )"; } // namespace @@ -3464,9 +3491,11 @@ void main() { ssb.sum = imageLoad(i0, ivec2(2, 3)).r + imageLoad(i1, ivec3(1, 1, EXPECT_NE(essl.find("ivec3(2, 0, 3)"), String::npos) << essl; } -// Scope, half one: a NON-arrayed 1D storage image is emitted correctly by the very same -// SPIRV-Cross code, so the pass must not touch it - replacing working emission with our own buys -// nothing and risks everything. +// Scope, half one: a NON-arrayed 1D storage image that is only READ or WRITTEN is emitted +// correctly by the very same SPIRV-Cross code, so the pass must not touch it - replacing working +// emission with our own buys nothing and risks everything. (The atomic shape below is the one +// exception, and it is gated on an OpImageTexelPointer actually being present, which is why this +// fixture still passes through byte for byte.) TEST_F(ProgramUtilTest, Lower1DArrayImagesLeavesNonArrayed1DImagesToSpirvCross) { using namespace MG_Util::ShaderTranspiler; @@ -3489,6 +3518,94 @@ void main() { ssb.sum = imageLoad(i0, 2).r; } << "SPIRV-Cross's own 1D-as-2D emulation must still be what handles this:\n" << essl; } +// The negative control for the ATOMIC half, and the reason the non-arrayed case is in scope at +// all: SPIRV-Cross widens a 1D image coordinate in OpImageRead and OpImageWrite but not in +// OpImageTexelPointer, so the atomic comes out addressing an `uimage2D` with a scalar. Every ES +// driver answers "no matching overloaded function found" and the whole stage - with every other +// image in it - is lost. Pinning the upstream behaviour here means a future SPIRV-Cross bump that +// fixes it fails this test instead of leaving the lowering as silent dead weight. +TEST_F(ProgramUtilTest, SpirvCrossEmitsAScalarCoordinateForA1DImageAtomic) { + using namespace MG_Util::ShaderTranspiler; + + const Vector spirv = BuildSpirvForStage(k1DImageAtomicCompute, GL_COMPUTE_SHADER); + ASSERT_FALSE(spirv.empty()); + ASSERT_EQ(Count1DNonArrayedStorageImageTypes(spirv), 1u) + << "glslang no longer emits a Dim1D/non-arrayed/Sampled=2 image for uimage1D"; + + const String essl = DecompileToEssl(spirv); + ASSERT_FALSE(essl.empty()); + EXPECT_NE(essl.find("uimage2D"), String::npos) + << "SPIRV-Cross declares the 1D image as 2D on ES; that half it does do:\n" << essl; + EXPECT_NE(essl.find("imageAtomicAdd(i0, 2"), String::npos) + << "SPIRV-Cross is expected to pass the SCALAR coordinate straight through to the atomic. " + "If this no longer happens, the non-arrayed half of Lower1DArrayImagesForEssl may no " + "longer be needed:\n" + << essl; + EXPECT_EQ(essl.find("ivec2("), String::npos) + << "nothing else in this fixture builds an ivec2, so its absence is the defect:\n" << essl; +} + +// The fix: the type becomes a plain 2D image - which is what MobileGL stores a GL_TEXTURE_1D in, +// height 1 - and the coordinate becomes (u, 0), so the atomic type-checks against the declaration +// SPIRV-Cross was already emitting. +TEST_F(ProgramUtilTest, Lower1DArrayImagesWidensThe1DAtomicCoordinate) { + using namespace MG_Util::ShaderTranspiler; + + const Vector raw = BuildSpirvForStage(k1DImageAtomicCompute, GL_COMPUTE_SHADER); + ASSERT_FALSE(raw.empty()); + + Vector spirv; + ASSERT_TRUE(ShaderCompiler::SanitizeAndOptimizeBinary(raw, spirv)); + ASSERT_EQ(Count1DNonArrayedStorageImageTypes(spirv), 1u) + << "the shared chain must leave the 1D image for this pass to handle"; + + const Uint64 failuresBefore = ShaderCompiler::SpirvValidationFailureCount(); + + Vector lowered; + ASSERT_TRUE(ShaderCompiler::Lower1DArrayImagesForEssl(spirv, lowered, true)); + ASSERT_FALSE(lowered.empty()); + + EXPECT_EQ(Count1DNonArrayedStorageImageTypes(lowered), 0u) + << "no non-arrayed 1D storage image type may survive when an atomic reaches one:\n" + << DisassembleSpirv(lowered); + EXPECT_EQ(ShaderCompiler::SpirvValidationFailureCount(), failuresBefore) + << "the lowered module must stay validator-clean"; + + const String essl = DecompileToEssl(lowered); + ASSERT_FALSE(essl.empty()); + EXPECT_NE(essl.find("uimage2D"), String::npos) + << "the declaration must still be the 2D one the ES texture is:\n" << essl; + EXPECT_NE(essl.find("imageAtomicAdd(i0, ivec2(2, 0)"), String::npos) + << "the atomic must address the image with the same (u, 0) SPIRV-Cross writes for a read " + "or a write:\n" + << essl; +} + +// The declined shape for the atomic half, for the same reason as the arrayed one: after the +// rewrite the image is 2D, so imageSize() yields two components where the shader consumes one and +// there is no correct scalar to substitute. +TEST_F(ProgramUtilTest, Lower1DArrayImagesDeclinesA1DAtomicModuleThatQueriesTheImageSize) { + using namespace MG_Util::ShaderTranspiler; + + const Vector spirv = BuildSpirvForStage(R"(#version 440 core +layout (local_size_x = 1) in; +layout (r32ui) coherent uniform uimage1D i0; +layout (std430, binding = 0) buffer SSB { uint sum; } ssb; +void main() { ssb.sum = imageAtomicAdd(i0, 2, 7u) + uint(imageSize(i0)); } +)", + GL_COMPUTE_SHADER); + ASSERT_FALSE(spirv.empty()); + const auto traits = Lower1DArrayImagesPass::InspectBinary(spirv); + ASSERT_TRUE(traits.declaresImage && traits.queriesImageSize) + << "the fixture must contain the shape the pass declines"; + + Vector lowered; + ASSERT_TRUE(ShaderCompiler::Lower1DArrayImagesForEssl(spirv, lowered, true)); + EXPECT_EQ(lowered, spirv) << "a declined module must be handed back untouched, not partly rewritten"; + EXPECT_EQ(Count1DNonArrayedStorageImageTypes(lowered), 1u) + << "declining means the 1D type is still there for the driver to reject"; +} + // Scope, half two: a 1D-array SAMPLER reaches SPIRV-Cross's sampler path, which does check // `arrayed` and does move the layer into the third component. The pass is storage-image only. TEST_F(ProgramUtilTest, Lower1DArrayImagesLeavesSampledImagesAlone) { @@ -3947,6 +4064,49 @@ TEST_F(ProgramUtilTest, StorageBlockBindingCeilingIsCheckedAtItsExactBoundary) { EXPECT_FALSE(FindShaderStorageBindingViolation("layout(binding = 36) buffer B { int x; };\n", 0).has_value()); } +// KHR-GL43.shader_atomic_counters.negative-offset-1: an atomic counter whose layout(offset = N) +// puts its last byte past GL_MAX_ATOMIC_COUNTER_BUFFER_SIZE is a COMPILE-time error, and the CTS +// never links the shader at all. MobileGL only had the rule at link, because the Vulkan-relaxed +// parse never reaches glslang's fixOffset(). +TEST_F(ProgramUtilTest, AtomicCounterOffsetCeilingIsCheckedAtCompile) { + using namespace MG_Util::ShaderTranspiler; + + const auto violation = [](const String& body) { + return FindAtomicCounterOffsetViolation("#version 430 core\n" + body + "void main() {}\n"); + }; + const String maxSize = std::to_string(MAX_ATOMIC_COUNTER_BUFFER_SIZE); + const String lastLegal = std::to_string(MAX_ATOMIC_COUNTER_BUFFER_SIZE - 4); + + // The boundary itself: the last counter that still fits, and the first that does not. + EXPECT_FALSE(violation("layout(binding = 0, offset = " + lastLegal + ") uniform atomic_uint c;\n").has_value()); + EXPECT_TRUE(violation("layout(binding = 0, offset = " + maxSize + ") uniform atomic_uint c;\n").has_value()); + + // An array occupies one word per element, so what has to fit is the LAST one. + EXPECT_FALSE(violation("layout(offset = " + std::to_string(MAX_ATOMIC_COUNTER_BUFFER_SIZE - 16) + + ") uniform atomic_uint c[4];\n") + .has_value()); + EXPECT_TRUE(violation("layout(offset = " + std::to_string(MAX_ATOMIC_COUNTER_BUFFER_SIZE - 8) + + ") uniform atomic_uint c[4];\n") + .has_value()); + + // An offset that is not a multiple of 4 (GL 4.6 core 7.7), and one that is. + EXPECT_TRUE(violation("layout(offset = 2) uniform atomic_uint c;\n").has_value()); + EXPECT_FALSE(violation("layout(offset = 8) uniform atomic_uint c;\n").has_value()); + + // Things the scanner must NOT judge: a counter with no explicit offset, an `offset` that is + // an ordinary identifier rather than a layout qualifier, an array sized by an expression, + // and an offset qualifier that belongs to a different declaration. + EXPECT_FALSE(violation("uniform atomic_uint c;\nconst int offset = 99999;\n").has_value()); + EXPECT_FALSE(violation("const int kCount = 4;\nlayout(offset = " + maxSize + + ") uniform atomic_uint c[kCount];\n") + .has_value()); + EXPECT_FALSE(violation("layout(offset = " + maxSize + ") uniform Block { int x; };\n" + "uniform atomic_uint c;\n") + .has_value()); + // A source with no counter at all never pays for the scan and never reports one. + EXPECT_FALSE(FindAtomicCounterOffsetViolation("#version 430 core\nvoid main() {}\n").has_value()); +} + // KHR-GL43.explicit_uniform_location.uniform-loc-nondecimal: GLSL integer literals are C-style, so // layout(location = 0xA) is 10 and layout(location = 010) is OCTAL 8. The extractor used to accept // a base-10 digit run and nothing else: the hex spelling failed the test entirely and the diff --git a/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt b/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt index e38e5b43..3235ce07 100644 --- a/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt +++ b/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt @@ -12,6 +12,8 @@ add_executable( UniquifyIoBlockNamesTest.cpp LowerViewportIndexTest.cpp ClampMultisampleFetchTest.cpp + LegalizeStorageBlockArrayIndexTest.cpp + FlattenAtomicCounterBlockTest.cpp ) target_include_directories(SpirvPassTest PRIVATE diff --git a/MobileGL/MG_Test/ShaderTranspiler/FlattenAtomicCounterBlockTest.cpp b/MobileGL/MG_Test/ShaderTranspiler/FlattenAtomicCounterBlockTest.cpp new file mode 100644 index 00000000..05daba97 --- /dev/null +++ b/MobileGL/MG_Test/ShaderTranspiler/FlattenAtomicCounterBlockTest.cpp @@ -0,0 +1,214 @@ +// MobileGL - MobileGL/MG_Test/ShaderTranspiler/FlattenAtomicCounterBlockTest.cpp +// Copyright (c) 2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#include + +#define SPV_ENABLE_UTILITY_CODE +#include "glslang/SPIRV/spirv.hpp11" +#undef SPV_ENABLE_UTILITY_CODE + +#include "Includes.h" +#include +#include + +#include + +#include +#include +#include +#include + +using namespace MobileGL; +using MobileGL::MG_Util::ShaderTranspiler::ShaderCompiler; + +namespace { + constexpr SizeT kSpirvHeaderWordCount = 5u; + + template + void ForEachInstruction(const Vector& spirv, Visitor&& visit) { + for (SizeT offset = kSpirvHeaderWordCount; offset < spirv.size();) { + const Uint32 wordCount = spirv[offset] >> 16u; + if (wordCount == 0u || offset + wordCount > spirv.size()) break; + visit(static_cast(spirv[offset] & 0xffffu), &spirv[offset], wordCount); + offset += wordCount; + } + } + + Vector CompileCompute(const String& source) { + using namespace MobileGL::MG_Util::ShaderTranspiler; + ShaderAttrib shaderAttrib{.shaderType = GL_COMPUTE_SHADER, .sourceStr = source}; + auto shaderResult = ShaderCompiler::CompileShader(shaderAttrib); + EXPECT_TRUE(shaderResult) << (shaderResult ? String{} : shaderResult.error().log); + if (!shaderResult) return {}; + + ProgramAttrib programAttrib{.shaders = {shaderResult.value()}}; + auto programResult = ShaderCompiler::LinkProgram(programAttrib); + EXPECT_TRUE(programResult) << (programResult ? String{} : programResult.error().log); + if (!programResult) return {}; + + ProgramBinaryAttrib binaryAttrib{.shaderTypes = {GL_COMPUTE_SHADER}, .program = *programResult.value()}; + auto binaryResult = ShaderCompiler::GetSpirvBinaryFromProgram(binaryAttrib); + EXPECT_TRUE(binaryResult) << (binaryResult ? String{} : binaryResult.error().log); + if (!binaryResult || binaryResult->empty()) return {}; + return binaryResult->front(); + } + + bool Validates(const Vector& spirv) { + spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_1); + tools.SetMessageConsumer( + [](spv_message_level_t, const char*, const spv_position_t& position, const char* message) { + ADD_FAILURE() << "spirv-val at word " << position.index << ": " << message; + }); + return tools.Validate(spirv); + } + + // Test-side reference walker, deliberately independent of the production code. + Uint32 FindAtomicCounterBlockStructId(const Vector& spirv) { + const String prefix = MG_Util::ShaderTranspiler::ATOMIC_COUNTER_BLOCK_PREFIX; + Uint32 structId = 0; + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + if (opcode != spv::Op::OpName || wordCount < 3u || structId != 0u) return; + const char* text = reinterpret_cast(&words[2]); + const SizeT available = static_cast(wordCount - 2u) * sizeof(Uint32); + if (available < prefix.size()) return; + if (std::strncmp(text, prefix.c_str(), prefix.size()) != 0) return; + structId = words[1]; + }); + return structId; + } + + // The Offset of member `member` on struct `structId`, or -1. + Int64 MemberOffsetOf(const Vector& spirv, Uint32 structId, Uint32 member) { + Int64 offset = -1; + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + if (opcode != spv::Op::OpMemberDecorate || wordCount < 5u) return; + if (words[1] != structId || words[2] != member) return; + if (static_cast(words[3]) != spv::Decoration::Offset) return; + offset = words[4]; + }); + return offset; + } + + Uint32 MemberCountOf(const Vector& spirv, Uint32 structId) { + Uint32 count = 0; + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + if (opcode != spv::Op::OpTypeStruct || wordCount < 2u || words[1] != structId) return; + count = wordCount - 2u; + }); + return count; + } + + Uint32 MemberTypeOf(const Vector& spirv, Uint32 structId, Uint32 member) { + Uint32 typeId = 0; + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + if (opcode != spv::Op::OpTypeStruct || wordCount < 3u + member || words[1] != structId) return; + typeId = words[2 + member]; + }); + return typeId; + } + + // The declared length of an OpTypeArray, resolved through the uint constants in the module. + Int64 ArrayLengthOf(const Vector& spirv, Uint32 arrayTypeId) { + std::map constants; + Int64 length = -1; + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + if (opcode == spv::Op::OpConstant && wordCount >= 4u) constants[words[2]] = words[3]; + if (opcode == spv::Op::OpTypeArray && wordCount >= 4u && words[1] == arrayTypeId) { + const auto it = constants.find(words[3]); + if (it != constants.end()) length = it->second; + } + }); + return length; + } + + // KHR-GL43.compute_shader.resources-atomic-counter's non-zero-offset shape: two counters + // declared eight bytes into the buffer, which glslang lowers to one block member at Offset 8. + constexpr const char* kOffsetCounters = R"(#version 450 core +layout(local_size_x = 1) in; +layout(binding = 1, offset = 8) uniform atomic_uint g_counter[2]; +layout(std430, binding = 0) buffer Output { uint value[]; } g_out; +void main() { + g_out.value[0] = atomicCounterIncrement(g_counter[0]); + g_out.value[1] = atomicCounterIncrement(g_counter[1]); +} +)"; + + // The latch: offset 0 is what nearly every shader declares, and it transpiles today. + constexpr const char* kNaturalCounters = R"(#version 450 core +layout(local_size_x = 1) in; +layout(binding = 1, offset = 0) uniform atomic_uint g_counter[2]; +layout(std430, binding = 0) buffer Output { uint value[]; } g_out; +void main() { + g_out.value[0] = atomicCounterIncrement(g_counter[0]); + g_out.value[1] = atomicCounterIncrement(g_counter[1]); +} +)"; + + constexpr const char* kNoCounters = R"(#version 450 core +layout(local_size_x = 1) in; +layout(std430, binding = 0) buffer Output { uint value[]; } g_out; +void main() { + g_out.value[0] = 1u; +} +)"; +} // namespace + +TEST(FlattenAtomicCounterBlockPass, MovesTheBlockToOffsetZeroAndGrowsTheArray) { + const Vector input = CompileCompute(kOffsetCounters); + ASSERT_FALSE(input.empty()); + const Uint32 structId = FindAtomicCounterBlockStructId(input); + ASSERT_NE(structId, 0u) << "glslang did not lower the counters onto a gl_AtomicCounterBlock_*"; + ASSERT_EQ(MemberOffsetOf(input, structId, 0u), 8) << "the input's member 0 is not at the declared offset"; + + Vector output; + ASSERT_TRUE(ShaderCompiler::FlattenAtomicCounterBlockOffsetsForEssl(input, output, true)); + ASSERT_FALSE(output.empty()); + + const Uint32 outStructId = FindAtomicCounterBlockStructId(output); + ASSERT_EQ(outStructId, structId) << "the block's id must not move; SetAtomicCounterBlockBindings " + "still finds it by name"; + EXPECT_EQ(MemberCountOf(output, outStructId), 1u); + EXPECT_EQ(MemberOffsetOf(output, outStructId, 0u), 0) + << "member 0 must sit at offset 0 or no std140/std430 layout can express the block"; + // Two counters eight bytes in: the flattened array has to cover bytes [0, 16), i.e. 4 uints, + // so counter k lands on element 2 + k and therefore on byte 8 + 4k - where it was declared. + EXPECT_EQ(ArrayLengthOf(output, MemberTypeOf(output, outStructId, 0u)), 4); + EXPECT_TRUE(Validates(output)); +} + +TEST(FlattenAtomicCounterBlockPass, LeavesANaturallyPackedBlockByteIdentical) { + const Vector input = CompileCompute(kNaturalCounters); + ASSERT_FALSE(input.empty()); + ASSERT_NE(FindAtomicCounterBlockStructId(input), 0u); + + Vector output; + ASSERT_TRUE(ShaderCompiler::FlattenAtomicCounterBlockOffsetsForEssl(input, output, true)); + EXPECT_EQ(output, input); +} + +TEST(FlattenAtomicCounterBlockPass, LeavesAShaderWithoutCountersByteIdentical) { + const Vector input = CompileCompute(kNoCounters); + ASSERT_FALSE(input.empty()); + + Vector output; + ASSERT_TRUE(ShaderCompiler::FlattenAtomicCounterBlockOffsetsForEssl(input, output, true)); + EXPECT_EQ(output, input); +} + +TEST(FlattenAtomicCounterBlockPass, IsIdempotent) { + const Vector input = CompileCompute(kOffsetCounters); + ASSERT_FALSE(input.empty()); + + Vector once; + ASSERT_TRUE(ShaderCompiler::FlattenAtomicCounterBlockOffsetsForEssl(input, once, true)); + ASSERT_FALSE(once.empty()); + + Vector twice; + ASSERT_TRUE(ShaderCompiler::FlattenAtomicCounterBlockOffsetsForEssl(once, twice, true)); + EXPECT_EQ(twice, once); +} diff --git a/MobileGL/MG_Test/ShaderTranspiler/LegalizeStorageBlockArrayIndexTest.cpp b/MobileGL/MG_Test/ShaderTranspiler/LegalizeStorageBlockArrayIndexTest.cpp new file mode 100644 index 00000000..f4c7bfbd --- /dev/null +++ b/MobileGL/MG_Test/ShaderTranspiler/LegalizeStorageBlockArrayIndexTest.cpp @@ -0,0 +1,251 @@ +// MobileGL - MobileGL/MG_Test/ShaderTranspiler/LegalizeStorageBlockArrayIndexTest.cpp +// Copyright (c) 2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#include + +#define SPV_ENABLE_UTILITY_CODE +#include "glslang/SPIRV/spirv.hpp11" +#undef SPV_ENABLE_UTILITY_CODE + +#include "Includes.h" +#include +#include + +#include + +#include +#include + +using namespace MobileGL; +using MobileGL::MG_Util::ShaderTranspiler::ShaderCompiler; + +namespace { + constexpr SizeT kSpirvHeaderWordCount = 5u; + + template + void ForEachInstruction(const Vector& spirv, Visitor&& visit) { + for (SizeT offset = kSpirvHeaderWordCount; offset < spirv.size();) { + const Uint32 wordCount = spirv[offset] >> 16u; + if (wordCount == 0u || offset + wordCount > spirv.size()) break; + visit(static_cast(spirv[offset] & 0xffffu), &spirv[offset], wordCount); + offset += wordCount; + } + } + + Vector CompileCompute(const String& source) { + using namespace MobileGL::MG_Util::ShaderTranspiler; + ShaderAttrib shaderAttrib{.shaderType = GL_COMPUTE_SHADER, .sourceStr = source}; + auto shaderResult = ShaderCompiler::CompileShader(shaderAttrib); + EXPECT_TRUE(shaderResult) << (shaderResult ? String{} : shaderResult.error().log); + if (!shaderResult) return {}; + + ProgramAttrib programAttrib{.shaders = {shaderResult.value()}}; + auto programResult = ShaderCompiler::LinkProgram(programAttrib); + EXPECT_TRUE(programResult) << (programResult ? String{} : programResult.error().log); + if (!programResult) return {}; + + ProgramBinaryAttrib binaryAttrib{.shaderTypes = {GL_COMPUTE_SHADER}, .program = *programResult.value()}; + auto binaryResult = ShaderCompiler::GetSpirvBinaryFromProgram(binaryAttrib); + EXPECT_TRUE(binaryResult) << (binaryResult ? String{} : binaryResult.error().log); + if (!binaryResult || binaryResult->empty()) return {}; + return binaryResult->front(); + } + + bool Validates(const Vector& spirv) { + spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_1); + tools.SetMessageConsumer( + [](spv_message_level_t, const char*, const spv_position_t& position, const char* message) { + ADD_FAILURE() << "spirv-val at word " << position.index << ": " << message; + }); + return tools.Validate(spirv); + } + + Uint32 CountOpcode(const Vector& spirv, spv::Op wanted) { + Uint32 count = 0u; + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32*, Uint32) { + if (opcode == wanted) ++count; + }); + return count; + } + + // Test-side reference walker, deliberately independent of the production detection so a + // bug in the pass cannot hide behind the same helper: true when some access chain rooted + // at an array-of-storage-blocks variable carries a non-constant FIRST index, which is + // exactly what the Qualcomm ES compiler refuses. + bool HasDynamicBlockArrayIndex(const Vector& spirv) { + std::set blockStructs; // OpTypeStruct ids decorated Block / BufferBlock + std::set constants; // OpConstant / OpConstantNull result ids + std::set blockArrayTypes; // OpTypeArray ids whose element is such a struct + std::set blockArrayPointers;// OpTypePointer ids pointing at one of those arrays + std::set blockArrayVars; // OpVariable ids of one of those pointer types + + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + switch (opcode) { + case spv::Op::OpDecorate: + if (wordCount >= 3u) { + const auto decoration = static_cast(words[2]); + if (decoration == spv::Decoration::Block || + decoration == spv::Decoration::BufferBlock) { + blockStructs.insert(words[1]); + } + } + break; + case spv::Op::OpConstant: + if (wordCount >= 3u) constants.insert(words[2]); + break; + case spv::Op::OpConstantNull: + if (wordCount >= 3u) constants.insert(words[2]); + break; + case spv::Op::OpTypeArray: + // OpTypeArray + if (wordCount >= 4u && blockStructs.count(words[2]) != 0u) { + blockArrayTypes.insert(words[1]); + } + break; + case spv::Op::OpTypePointer: + // OpTypePointer + if (wordCount >= 4u && blockArrayTypes.count(words[3]) != 0u) { + blockArrayPointers.insert(words[1]); + } + break; + case spv::Op::OpVariable: + // OpVariable + if (wordCount >= 4u && blockArrayPointers.count(words[1]) != 0u) { + blockArrayVars.insert(words[2]); + } + break; + default: + break; + } + }); + + bool dynamic = false; + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + if (opcode != spv::Op::OpAccessChain && opcode != spv::Op::OpInBoundsAccessChain) return; + // OpAccessChain ... + if (wordCount < 5u) return; + if (blockArrayVars.count(words[3]) == 0u) return; + if (constants.count(words[4]) != 0u) return; + dynamic = true; + }); + return dynamic; + } + + // `for (i = 0; i < 4; ++i)` over an array of storage blocks - the shape + // KHR-GL43.shader_storage_buffer_object.basic-stdLayout-case1 uses. Foldable: the + // induction variable is a literal after unrolling. + constexpr const char* kLoopIndexedBlockArray = R"(#version 450 core +layout(local_size_x = 1) in; +layout(std430, binding = 0) buffer Blk { uint data[4]; } g_blocks[4]; +layout(std430, binding = 8) buffer Out { uint data[4]; } g_out; +void main() { + for (int i = 0; i < 4; ++i) { + g_out.data[i] = g_blocks[i].data[0]; + } +} +)"; + + // A uniform-sourced index - the shape + // KHR-GL43.shader_storage_buffer_object.advanced-indirectAddressing-case2 uses. Nothing + // can fold it, so the switch/select lowering is what has to carry it. + constexpr const char* kUniformIndexedBlockArray = R"(#version 450 core +layout(local_size_x = 1) in; +layout(std430, binding = 0) buffer Blk { uint data[4]; } g_blocks[4]; +layout(std430, binding = 8) buffer Out { uint value; } g_out; +uniform int g_index; +void main() { + g_blocks[g_index].data[0] = 7u; + g_out.value = g_blocks[g_index].data[1]; +} +)"; + + // The positive control from the device run: dynamic addressing through an array MEMBER of + // ONE block is legal ES and must not be rewritten. + constexpr const char* kArrayMemberInsideOneBlock = R"(#version 450 core +layout(local_size_x = 1) in; +layout(std430, binding = 0) buffer Blk { uint data[4]; } g_block; +layout(std430, binding = 8) buffer Out { uint value; } g_out; +uniform int g_index; +void main() { + g_out.value = g_block.data[g_index]; +} +)"; + + // A block array indexed only with literals is already legal ES. + constexpr const char* kConstantIndexedBlockArray = R"(#version 450 core +layout(local_size_x = 1) in; +layout(std430, binding = 0) buffer Blk { uint data[4]; } g_blocks[4]; +layout(std430, binding = 8) buffer Out { uint value; } g_out; +void main() { + g_out.value = g_blocks[2].data[0] + g_blocks[3].data[1]; +} +)"; +} // namespace + +TEST(LegalizeStorageBlockArrayIndexPass, FoldsALoopIndexedBlockArray) { + const Vector input = CompileCompute(kLoopIndexedBlockArray); + ASSERT_FALSE(input.empty()); + EXPECT_TRUE(HasDynamicBlockArrayIndex(input)); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl(input, output, true)); + ASSERT_FALSE(output.empty()); + // Either half of the legalization is an acceptable outcome here - what the ES driver + // cares about is only that no dynamic subscript survives. + EXPECT_FALSE(HasDynamicBlockArrayIndex(output)); + EXPECT_TRUE(Validates(output)); +} + +TEST(LegalizeStorageBlockArrayIndexPass, LowersAUniformIndexedWriteToASwitchAndAReadToSelects) { + const Vector input = CompileCompute(kUniformIndexedBlockArray); + ASSERT_FALSE(input.empty()); + EXPECT_TRUE(HasDynamicBlockArrayIndex(input)); + EXPECT_EQ(CountOpcode(input, spv::Op::OpSwitch), 0u); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl(input, output, true)); + ASSERT_FALSE(output.empty()); + EXPECT_FALSE(HasDynamicBlockArrayIndex(output)); + // One switch for the store, and one select per element past the first for the load. + EXPECT_EQ(CountOpcode(output, spv::Op::OpSwitch), 1u); + EXPECT_EQ(CountOpcode(output, spv::Op::OpSelect), 3u); + EXPECT_TRUE(Validates(output)); +} + +TEST(LegalizeStorageBlockArrayIndexPass, LeavesADynamicMemberOfOneBlockByteIdentical) { + const Vector input = CompileCompute(kArrayMemberInsideOneBlock); + ASSERT_FALSE(input.empty()); + EXPECT_FALSE(HasDynamicBlockArrayIndex(input)); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl(input, output, true)); + EXPECT_EQ(output, input); +} + +TEST(LegalizeStorageBlockArrayIndexPass, LeavesAConstantIndexedBlockArrayByteIdentical) { + const Vector input = CompileCompute(kConstantIndexedBlockArray); + ASSERT_FALSE(input.empty()); + EXPECT_FALSE(HasDynamicBlockArrayIndex(input)); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl(input, output, true)); + EXPECT_EQ(output, input); +} + +TEST(LegalizeStorageBlockArrayIndexPass, IsIdempotent) { + const Vector input = CompileCompute(kUniformIndexedBlockArray); + ASSERT_FALSE(input.empty()); + + Vector once; + ASSERT_TRUE(ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl(input, once, true)); + ASSERT_FALSE(once.empty()); + + Vector twice; + ASSERT_TRUE(ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl(once, twice, true)); + EXPECT_EQ(twice, once); +} diff --git a/MobileGL/MG_Test/State/RenderStateTest.cpp b/MobileGL/MG_Test/State/RenderStateTest.cpp index 66ed6033..b4a2a7f7 100644 --- a/MobileGL/MG_Test/State/RenderStateTest.cpp +++ b/MobileGL/MG_Test/State/RenderStateTest.cpp @@ -559,3 +559,76 @@ TEST_F(RenderStateTest, IndexedRectangleQueriesRejectAnOutOfRangeIndex) { MG_Impl::GLImpl::GetDoublei_v(GL_DEPTH_RANGE, kMaxViewports - 1, doubles); ExpectSingleGlError(GL_NO_ERROR); } + +// --------------------------------------------------------------------------------------------- +// "Has the application written this scissor rectangle?" - the flag, not the extent +// --------------------------------------------------------------------------------------------- +// glScissor(0, 0, 0, 0) is legal GL and means "the scissor test rejects every fragment", but it +// is byte-identical to the never-written default, whose meaning is the opposite ("the whole +// window", which the frontend cannot spell before a surface exists). DirectGLES resolved the two +// by looking at the EXTENT and so inverted every deliberately empty box into the full surface - +// KHR-GL43.viewport_array.scissor_zero_dimension is exactly that draw, and it came back holding +// the drawn colour where the untouched fill was required. +// +// These drive RenderState directly instead of the GL entry points on purpose: the flag's whole +// content is what it says BEFORE the first scissor call of a context, and this binary shares one +// context across every case in the file, so a pristine object is the only place that state +// still exists by the time these run. + +namespace { + constexpr Uint32 kAllViewportsWritten = + RenderStateParameters::MAX_VIEWPORTS >= 32 ? ~0u : (1u << RenderStateParameters::MAX_VIEWPORTS) - 1u; +} // namespace + +TEST_F(RenderStateTest, AnEmptyScissorBoxIsDistinguishableFromNeverHavingBeenWritten) { + MG_State::GLState::RenderState state; + EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, 0u) + << "a fresh context has never been given a scissor box, and the all-zero rectangle it " + "starts with must not be mistaken for one"; + + // Not one stored byte moves here - every box already held (0,0,0,0) - and yet this is the + // call that turns "the frontend does not know the window size" into "reject every fragment". + state.SetScissorBox(IntVec4(0, 0, 0, 0)); + EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, kAllViewportsWritten); + for (GLuint index = 0; index < kMaxViewports; ++index) { + EXPECT_EQ(state.GetScissorBoxIndexed(index), IntVec4(0, 0, 0, 0)) << "index " << index; + } +} + +TEST_F(RenderStateTest, AnIndexedScissorWriteClaimsOnlyItsOwnIndex) { + MG_State::GLState::RenderState state; + state.SetScissorBoxIndexed(5, IntVec4(0, 0, 0, 0)); + EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, 1u << 5); + + state.SetScissorBoxIndexed(0, IntVec4(0, 0, 0, 0)); + EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, (1u << 5) | 1u); + + // ARB_viewport_array makes the non-indexed setter a write to every index, so it claims all 16. + state.SetScissorBox(IntVec4(1, 2, 3, 4)); + EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, kAllViewportsWritten); +} + +TEST_F(RenderStateTest, TheFirstScissorWriteBumpsTheVersionEvenWhenTheValueDoesNotMove) { + // Load-bearing, and not merely tidy: DirectGLES' SyncRenderState early-outs on an unchanged + // render-state version BEFORE it reaches the span memcmp that would otherwise notice the + // flag. A version-less transition would sit in the parameter block, never be pushed, and the + // empty box would go on rendering as the whole surface. + MG_State::GLState::RenderState state; + const Uint initial = state.GetVersion(); + state.SetScissorBox(IntVec4(0, 0, 0, 0)); + EXPECT_GT(state.GetVersion(), initial) << "claiming the rectangle is itself a state change"; + + // Once claimed, a genuinely redundant write stays free - the flag costs one transition, not + // a version bump per call. + const Uint settled = state.GetVersion(); + state.SetScissorBox(IntVec4(0, 0, 0, 0)); + EXPECT_EQ(state.GetVersion(), settled); + + MG_State::GLState::RenderState indexed; + const Uint indexedInitial = indexed.GetVersion(); + indexed.SetScissorBoxIndexed(3, IntVec4(0, 0, 0, 0)); + EXPECT_GT(indexed.GetVersion(), indexedInitial); + const Uint indexedSettled = indexed.GetVersion(); + indexed.SetScissorBoxIndexed(3, IntVec4(0, 0, 0, 0)); + EXPECT_EQ(indexed.GetVersion(), indexedSettled); +} diff --git a/MobileGL/MG_Test/Texture/TextureTest.cpp b/MobileGL/MG_Test/Texture/TextureTest.cpp index 0c17236d..8bdfc511 100644 --- a/MobileGL/MG_Test/Texture/TextureTest.cpp +++ b/MobileGL/MG_Test/Texture/TextureTest.cpp @@ -4982,10 +4982,14 @@ TEST_F(TextureTest, CopyImageSubDataCountsCubeMapFacesOnTheZAxis) { ExpectSingleGlError(GL_INVALID_VALUE); } -// The other axis convention: GL puts a 1D ARRAY's layers on y for this entry point (srcY is the -// first layer, srcHeight the layer count), which is also where this frontend keeps them - so the -// level extent answers directly and z stays a single slice. -TEST_F(TextureTest, CopyImageSubDataBoundsA1DArraysLayersOnTheYAxis) { +// The other axis convention, and it is NOT the one this frontend stores. GL 4.6 core 18.3.2 +// treats every array texture as a stack of slices on Z and gives a 1D array an image HEIGHT OF +// ONE (which is exactly what the CTS asserts: it forces height = 1 for GL_TEXTURE_1D_ARRAY and +// lists the target as multilayer). MobileGL keeps a 1D array's layers on y internally - that is +// what glTexImage2D(GL_TEXTURE_1D_ARRAY, w, layers) writes - so this entry point has to convert, +// and measuring srcY against the LAYER count is what let an out-of-range srcY come back +// GL_NO_ERROR (KHR-GL43.copy_image.exceeding_boundaries, the src_test_case y variants). +TEST_F(TextureTest, CopyImageSubDataBoundsA1DArraysLayersOnTheZAxis) { const ScopedTextureBackendFunctionsOverride backendGuard; MG_Backend::gBackendFunctionsTable.GL.CopyImageSubData = RecordCopyImageSubData; g_copyImageSubDataCall = {}; @@ -5006,14 +5010,33 @@ TEST_F(TextureTest, CopyImageSubDataBoundsA1DArraysLayersOnTheYAxis) { GTEST_SKIP() << "this context could not give the 1D arrays storage"; } - MG_Impl::GLImpl::CopyImageSubData(srcTexture, GL_TEXTURE_1D_ARRAY, 0, 0, 3, 0, dstTexture, GL_TEXTURE_1D_ARRAY, - 0, 0, 3, 0, 4, 5, 1); + // 16 wide, 8 layers. Five layers from layer 3 is legal, and it is spelled on z with a + // height of 1 - the layer count rides on srcDepth. + MG_Impl::GLImpl::CopyImageSubData(srcTexture, GL_TEXTURE_1D_ARRAY, 0, 0, 0, 3, dstTexture, GL_TEXTURE_1D_ARRAY, + 0, 0, 0, 3, 4, 1, 5); EXPECT_TRUE(g_copyImageSubDataCall.Called); EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + // One layer past the last one is out of bounds on z. g_copyImageSubDataCall = {}; - MG_Impl::GLImpl::CopyImageSubData(srcTexture, GL_TEXTURE_1D_ARRAY, 0, 0, 4, 0, dstTexture, GL_TEXTURE_1D_ARRAY, - 0, 0, 0, 0, 4, 5, 1); + MG_Impl::GLImpl::CopyImageSubData(srcTexture, GL_TEXTURE_1D_ARRAY, 0, 0, 0, 4, dstTexture, GL_TEXTURE_1D_ARRAY, + 0, 0, 0, 0, 4, 1, 5); + EXPECT_FALSE(g_copyImageSubDataCall.Called); + ExpectSingleGlError(GL_INVALID_VALUE); + + // The image is one texel HIGH whatever its layer count is, so any srcY past 0 is out of + // bounds - this is the KHR-GL43.copy_image case that used to be measured against the 8 + // layers and pass. + g_copyImageSubDataCall = {}; + MG_Impl::GLImpl::CopyImageSubData(srcTexture, GL_TEXTURE_1D_ARRAY, 0, 0, 6, 0, dstTexture, GL_TEXTURE_1D_ARRAY, + 0, 0, 6, 0, 4, 1, 1); + EXPECT_FALSE(g_copyImageSubDataCall.Called); + ExpectSingleGlError(GL_INVALID_VALUE); + + // ... and a height of 1 at y = 0 is the only legal y extent. + g_copyImageSubDataCall = {}; + MG_Impl::GLImpl::CopyImageSubData(srcTexture, GL_TEXTURE_1D_ARRAY, 0, 0, 0, 0, dstTexture, GL_TEXTURE_1D_ARRAY, + 0, 0, 0, 0, 4, 2, 1); EXPECT_FALSE(g_copyImageSubDataCall.Called); ExpectSingleGlError(GL_INVALID_VALUE); } diff --git a/MobileGL/MG_Util/SelfTest/DriverPost.cpp b/MobileGL/MG_Util/SelfTest/DriverPost.cpp index ab24e9d9..26d31e48 100644 --- a/MobileGL/MG_Util/SelfTest/DriverPost.cpp +++ b/MobileGL/MG_Util/SelfTest/DriverPost.cpp @@ -519,13 +519,12 @@ namespace MobileGL::MG_Util::SelfTest { "narrowed members, so an application that hard-codes std140 offsets " "computed for doubles must query them instead")); builder.Warn("64-bit vertex attributes", - "not supported (ES has no GL_DOUBLE vertex format, and after the fp64 demotion " - "above there is no 64-bit shader input left to feed either); " - "glVertexAttribLFormat / glVertexArrayAttribLFormat succeed and their state is " - "queryable, but an ENABLED 64-bit array is DROPPED at draw and the attribute " - "reads its generic current value - feed the attribute with " - "glVertexAttribPointer(GL_FLOAT) instead, which a demoted dvec input reads " - "correctly"); + "narrowed to float32 (ES has no GL_DOUBLE vertex format, and after the fp64 " + "demotion above there is no 64-bit shader input left to feed either); " + "glVertexAttribLFormat / glVertexArrayAttribLFormat succeed, their state is " + "queryable, and an ENABLED 64-bit array IS fetched - the source doubles are " + "deinterleaved into a float32 stream at draw, so values outside float32's " + "range or precision are rounded rather than exact"); if (glesFuncs.glPatchParameteri != nullptr) { builder.Pass("Tessellation patch parameters", "glPatchParameteri present (GL_PATCH_VERTICES reaches the driver)"); @@ -2337,12 +2336,12 @@ namespace MobileGL::MG_Util::SelfTest { "for doubles must query them instead", features.shaderFloat64 == VK_TRUE ? "supported" : "unsupported"))); builder.Warn("64-bit vertex attributes", - "not supported; there is no 64-bit shader input left to feed after the fp64 demotion " - "above, and no VK_FORMAT_R64*_SFLOAT vertex fetch to feed it with on most devices " - "anyway. glVertexAttribLFormat succeeds and its state is queryable, but an ENABLED " - "64-bit array is DROPPED at pipeline build and the attribute reads its generic " - "current value - feed the attribute with glVertexAttribPointer(GL_FLOAT) instead, " - "which a demoted dvec input reads correctly"); + "narrowed to float32; there is no 64-bit shader input left to feed after the fp64 " + "demotion above, and no VK_FORMAT_R64*_SFLOAT vertex fetch to feed it with on most " + "devices anyway. glVertexAttribLFormat succeeds, its state is queryable, and an " + "ENABLED 64-bit array IS fetched - the source doubles are deinterleaved into a " + "float32 stream at draw, so values outside float32's range or precision are " + "rounded rather than exact"); Bool shaderDrawParameters = false; if (vkGetPhysicalDeviceFeatures2Fn != nullptr && properties.apiVersion >= VK_API_VERSION_1_1) { diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index 613128b8..9c30ed6d 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -41,6 +41,8 @@ #include "SpirvPasses/StripNoPerspectivePass.h" #include "SpirvPasses/EmulateNoPerspectivePass.h" #include "SpirvPasses/LegalizeFragmentOutputIndexPass.h" +#include "SpirvPasses/LegalizeStorageBlockArrayIndexPass.h" +#include "SpirvPasses/FlattenAtomicCounterBlockPass.h" #include "spirv-tools/libspirv.h" #include "spirv-tools/optimizer.hpp" #include "source/opt/build_module.h" @@ -941,6 +943,100 @@ namespace MobileGL { return true; } + bool ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl( + const Vector& inputBinary, Vector& outputBinary, + const bool enableSpirvValidation) { + using namespace spvtools; + + // Detection gates everything: a module that declares no array of storage + // blocks, or indexes one only with constants - every shader but a handful - + // pays one BuildModule and is handed back byte for byte, so the folding chain + // can never perturb a shader that did not need it. + if (!LegalizeStorageBlockArrayIndexPass::BinaryHasDynamicStorageBlockArrayIndexing( + inputBinary)) { + outputBinary = inputBinary; + return true; + } + + // Stock passes do the real work, exactly as in the fragment-output + // legalization. The only bespoke member of the chain is the loop-control hint + // the stock unroller demands (see the pass header); with it set, the + // `for (i = 0; i < 4; ++i) arr[i]...` shape folds to literals here and the + // fallback below never runs. + Optimizer folder(SPV_ENV_VULKAN_1_1); + // First, because both the unroller and the marking pass below read the + // induction variable as an OpPhi, and glslang emits it as loads and stores of + // a Function variable. + folder.RegisterPass(CreateLocalMultiStoreElimPass()); + folder.RegisterPass(LegalizeStorageBlockArrayIndexPass::CreateMarkLoopsForUnrollPass()); + folder.RegisterPass(CreateLoopUnrollPass(true)); + // Fold the unrolled induction values into the access chains, then clear out + // what constant conditions leave behind. + folder.RegisterPass(CreateCCPPass()); + folder.RegisterPass(CreateSimplificationPass()); + folder.RegisterPass(CreateDeadBranchElimPass()); + folder.RegisterPass(CreateBlockMergePass()); + + Vector folded; + if (!RunOptimizerChecked("LegalizeStorageBlockArrayIndexingForEssl.fold", folder, + inputBinary, folded, true, enableSpirvValidation) || + folded.empty()) { + // Fail open onto the fallback rather than onto the illegal module. + folded = inputBinary; + } + + if (!LegalizeStorageBlockArrayIndexPass::BinaryHasDynamicStorageBlockArrayIndexing( + folded)) { + outputBinary = folded; + return true; + } + + // Genuinely dynamic (uniform-derived, non-constant trip count, ...): lower it. + Optimizer lowerer(SPV_ENV_VULKAN_1_1); + lowerer.RegisterPass( + LegalizeStorageBlockArrayIndexPass::CreateLowerToConstantSwitchPass()); + // The chains the lowering replaced are dead now; remove_outputs must stay + // false here for the same reason it does in SanitizeAndOptimizeBinary. + lowerer.RegisterPass(CreateAggressiveDCEPass(false)); + + if (!RunOptimizerChecked("LegalizeStorageBlockArrayIndexingForEssl.lower", lowerer, folded, + outputBinary, true, enableSpirvValidation) || + outputBinary.empty()) { + outputBinary = folded; + return true; + } + + if (LegalizeStorageBlockArrayIndexPass::BinaryHasDynamicStorageBlockArrayIndexing( + outputBinary)) { + // MGLOG_W, latched, for the same reason the fragment-output one is: this + // runs per shader compile and shader packs compile lazily mid-session. + MGLOG_W_ONCE("[spirv] LegalizeStorageBlockArrayIndexingForEssl: an array of storage " + "blocks is still indexed dynamically; a strict ES driver will reject " + "this shader"); + } + return true; + } + + bool ShaderCompiler::FlattenAtomicCounterBlockOffsetsForEssl( + const Vector& inputBinary, Vector& outputBinary, + const bool enableSpirvValidation) { + using namespace spvtools; + + // Detection gates everything: a module with no atomic counter, or one whose + // counters sit at their natural std430 offsets - which is every shader that omits + // the offset qualifier - pays one BuildModule and is handed back byte for byte. + if (!FlattenAtomicCounterBlockPass::BinaryHasOffsetAtomicCounterBlock(inputBinary)) { + outputBinary = inputBinary; + return true; + } + + Optimizer optimizer(SPV_ENV_VULKAN_1_1); + optimizer.RegisterPass(FlattenAtomicCounterBlockPass::CreateFlattenAtomicCounterBlockPass()); + + return RunOptimizerChecked("FlattenAtomicCounterBlockOffsetsForEssl", optimizer, inputBinary, + outputBinary, true, enableSpirvValidation); + } + bool ShaderCompiler::LowerRectImages(const Vector& inputBinary, Vector& outputBinary, const bool enableSpirvValidation) { @@ -956,26 +1052,27 @@ namespace MobileGL { using namespace spvtools; // Declined rather than half-translated: after the rewrite the image is a 2D - // array, so a size query on it yields three components where the shader consumes - // two. Handing back a differently-shaped size silently is worse than leaving the - // module alone and letting the driver say what it does not like - and unlike the - // access path there is no correct answer to substitute, because the ES texture + // (array) one, so a size query on it yields a component more than the shader + // consumes. Handing back a differently-shaped size silently is worse than leaving + // the module alone and letting the driver say what it does not like - and unlike + // the access path there is no correct answer to substitute, because the ES texture // genuinely has a height the GL one does not. // // MGLOG_W, latched: per shader compile, and shader packs compile lazily // mid-session. (Parked at MGLOG_I until the Log.h ordering fix made W live.) const auto traits = Lower1DArrayImagesPass::InspectBinary(inputBinary); // The overwhelmingly common answer, and the reason the inspection exists: no - // 1D-array storage image, so the module is handed back byte for byte without an - // Optimizer ever being built. Every ESSL shader in the process passes through - // here, so the cost of the case with nothing to do is the cost of this pass. + // 1D storage image this pass owns, so the module is handed back byte for byte + // without an Optimizer ever being built. Every ESSL shader in the process passes + // through here, so the cost of the case with nothing to do is the cost of this + // pass. if (!traits.declaresImage) { outputBinary = inputBinary; return true; } if (traits.queriesImageSize) { - MGLOG_W_ONCE("[spirv] Lower1DArrayImagesForEssl: the module queries the size of a 1D-array " - "storage image, which cannot be answered in the 2D-array shape ES stores it in; " + MGLOG_W_ONCE("[spirv] Lower1DArrayImagesForEssl: the module queries the size of a 1D " + "storage image, which cannot be answered in the 2D shape ES stores it in; " "leaving the module alone, and a strict ES driver will reject it"); outputBinary = inputBinary; return true; @@ -983,8 +1080,8 @@ namespace MobileGL { Optimizer optimizer(SPV_ENV_VULKAN_1_1); optimizer.RegisterPass(Lower1DArrayImagesPass::CreateLower1DArrayImagesPass()); - // Mandatory, not tidying. Rewriting a 1D-array image type to the 2D-array one - // makes it structurally IDENTICAL to any real 2D-array image of the same sampled + // Mandatory, not tidying. Rewriting a 1D(-array) image type to the 2D(-array) one + // makes it structurally IDENTICAL to any real 2D(-array) image of the same sampled // type and format that the module already declared - and SPIR-V forbids duplicate // non-aggregate type declarations, so the result fails validation. That collision // is not exotic: it is the shape of this whole change's headline case, where one diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h index ca27281c..f9d49220 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h @@ -156,6 +156,32 @@ namespace MobileGL { static bool LegalizeFragmentOutputIndexingForEssl(const Vector& inputBinary, Vector& outputBinary, bool enableSpirvValidation = false); + // Makes every index into an ARRAY OF SHADER STORAGE BLOCKS a constant integral + // expression. GL 4.3 allows any dynamically-uniform index there; the Qualcomm + // ES compiler enforces the ES 3.1 constant-expression rule and refuses the whole + // stage ("indexing into an SSBO array using a non-constant expression is not + // permitted"), which loses the program while the frontend still reports + // GL_LINK_STATUS = TRUE. Same two halves as the fragment-output legalization: + // fold the loop-derived indices, then lower whatever is genuinely dynamic to a + // switch over the array's range. DirectGLES transpile path only - Vulkan has no + // such restriction and must keep seeing one descriptor array. Copies the input + // through untouched when no block array is indexed dynamically, which is every + // shader but a handful. See LegalizeStorageBlockArrayIndexPass. + static bool LegalizeStorageBlockArrayIndexingForEssl(const Vector& inputBinary, + Vector& outputBinary, + bool enableSpirvValidation = false); + // Collapses each synthesized gl_AtomicCounterBlock_ into one uint array at + // offset 0, re-indexing every counter access to the element that used to sit at + // its byte offset. glslang preserves the application's layout(offset = N) as the + // member's Offset decoration, no std140/std430 layout can express a first member + // at a non-zero offset, and GLSL ES has no member layout(offset=) - so SPIRV-Cross + // throws and takes the whole stage with it. DirectGLES transpile path only. + // Copies the input through untouched when every counter block is already packed + // naturally, which is every shader that omits the offset qualifier. See + // FlattenAtomicCounterBlockPass. + static bool FlattenAtomicCounterBlockOffsetsForEssl(const Vector& inputBinary, + Vector& outputBinary, + bool enableSpirvValidation = false); // Rebases loads of the InstanceIndex builtin to (InstanceIndex - BaseInstance) so // shaders see GL's zero-based gl_InstanceID. Vertex shaders only; DirectVulkan // backend only (glslang's relaxed mode aliases gl_InstanceID to gl_InstanceIndex, @@ -168,10 +194,13 @@ namespace MobileGL { bool enableSpirvValidation = false); // GL_TEXTURE_1D_ARRAY storage images rewritten to the 2D-array shape the texture // is actually stored in on ES, with the layer moved from the coordinate's second - // component to its third. DirectGLES transpile path only - Vulkan binds a real - // VK_IMAGE_VIEW_TYPE_1D_ARRAY and must see the module unchanged. Copies the input - // through untouched when the module declares no such image, which is every shader - // but a handful. See Lower1DArrayImagesPass for what it declines and why. + // component to its third - and, when the module performs an image ATOMIC on one, + // the non-arrayed GL_TEXTURE_1D storage image to the 2D shape with its coordinate + // widened to (u, 0), which is the one 1D shape SPIRV-Cross does not widen itself. + // DirectGLES transpile path only - Vulkan binds a real VK_IMAGE_VIEW_TYPE_1D(_ARRAY) + // and must see the module unchanged. Copies the input through untouched when the + // module declares no such image, which is every shader but a handful. See + // Lower1DArrayImagesPass for what it declines and why. static bool Lower1DArrayImagesForEssl(const Vector& inputBinary, Vector& outputBinary, bool enableSpirvValidation = false); diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp index 1b15eb1e..6b1502f3 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include "EsslBuiltinFunctionNames.h" @@ -1603,6 +1604,92 @@ namespace MobileGL { return std::nullopt; } + std::optional FindAtomicCounterOffsetViolation(const String& source) { + // Fast path: both keywords are required for a violation to exist, and the pair is + // absent from every shader-pack source. + if (source.find("atomic_uint") == String::npos || source.find("offset") == String::npos) { + return std::nullopt; + } + + constexpr long long kAtomicCounterSize = 4; // one 32-bit word per counter + const long long maxBufferSize = static_cast(MAX_ATOMIC_COUNTER_BUFFER_SIZE); + const Vector tokens = TokenizeCode(source); + const SizeT count = tokens.size(); + // The offset the qualifier run currently being scanned declared, -1 for none. + // Same accumulate-then-consume shape as the storage-binding scan above. + long long offset = -1; + long long literal = 0; + for (SizeT pos = 0; pos < count; ++pos) { + const String& text = tokens[pos].text; + if (text == "layout" && pos + 1 < count && tokens[pos + 1].text == "(") { + SizeT j = pos + 2; + Int parenDepth = 1; + while (j < count && parenDepth > 0) { + const String& layoutToken = tokens[j].text; + if (layoutToken == "(") { + ++parenDepth; + } else if (layoutToken == ")") { + --parenDepth; + } else if (parenDepth == 1 && layoutToken == "offset" && j + 2 < count && + tokens[j + 1].text == "=" && + ParseGlslIntegerLiteral(tokens[j + 2].text, literal)) { + offset = literal; + j += 2; + } + ++j; + } + pos = j - 1; + continue; + } + if (text == "atomic_uint") { + // How far the declaration reaches: `atomic_uint c[N]` occupies N words + // from the offset. An unparsable or absent declarator (an expression-sized + // array, or the "layout(...) uniform atomic_uint;" default-qualifier form, + // which declares no counter at all) is left alone rather than guessed at - + // over-rejection here would be a compile failure the application cannot + // work around. + long long elements = 1; + SizeT k = pos + 1; + if (k < count && IsIdentifierToken(tokens[k])) { + ++k; + if (k < count && tokens[k].text == "[") { + elements = (k + 2 < count && tokens[k + 2].text == "]" && + ParseGlslIntegerLiteral(tokens[k + 1].text, literal)) + ? std::max(1, literal) + : -1; + } + } else { + elements = -1; + } + // Clamped so the byte arithmetic below cannot overflow on an absurd + // literal; any element count at or past the ceiling already fails. + elements = std::min(elements, maxBufferSize); + + if (offset >= 0 && elements > 0) { + if (offset % kAtomicCounterSize != 0) { + return "ERROR: invalid value " + std::to_string(offset) + + " for layout specifier 'offset': an atomic counter offset must be a " + "multiple of 4."; + } + if (offset > maxBufferSize - elements * kAtomicCounterSize) { + return "ERROR: invalid value " + std::to_string(offset) + + " for layout specifier 'offset': an atomic counter ending at byte " + + std::to_string(offset + elements * kAtomicCounterSize) + + " passes GL_MAX_ATOMIC_COUNTER_BUFFER_SIZE (" + + std::to_string(maxBufferSize) + ")."; + } + } + offset = -1; + continue; + } + // `uniform` and the precision/auxiliary qualifiers may sit between the layout + // list and the type keyword; anything else ends the run, so an offset never + // leaks onto an unrelated declaration. + if (text != "uniform" && !IsNonLayoutQualifierKeyword(text)) offset = -1; + } + return std::nullopt; + } + UnorderedMap ExtractExplicitUniformLocations(const String& source) { UnorderedMap locations; // Fast path: without the qualifier keyword there is nothing to extract. diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h index 0a55cc78..e7973a25 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h @@ -75,6 +75,18 @@ namespace MobileGL { // `maxBindings` is what glGetIntegerv answers for that pname; a non-positive value // means "nothing to check against" and every declaration passes. std::optional FindShaderStorageBindingViolation(const String& source, Int maxBindings); + + // GL 4.6 core 7.7 / ARB_shader_atomic_counters makes it a COMPILE-time error to + // declare an atomic counter at an offset that is not a multiple of 4, or whose last + // byte passes GL_MAX_ATOMIC_COUNTER_BUFFER_SIZE. glslang enforces both in fixOffset(), + // which the Vulkan-relaxed parse never reaches (vkRelaxedRemapUniformVariable folds + // the atomic_uint into a synthesized storage block and returns from declareVariable() + // first), so MobileGL only caught them at LINK - and + // KHR-GL43.shader_atomic_counters.negative-offset-1 never links at all. The + // cross-stage rule (two counters sharing a binding must not overlap) stays at link: + // a single-stage source cannot see it. Returns the compile-error text for the first + // violation, or nullopt for a clean source. + std::optional FindAtomicCounterOffsetViolation(const String& source); } // namespace ShaderTranspiler } // namespace MG_Util } // namespace MobileGL diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/DemoteFloat64Pass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/DemoteFloat64Pass.h index 73e6d822..3259f2cb 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/DemoteFloat64Pass.h +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/DemoteFloat64Pass.h @@ -50,6 +50,33 @@ namespace MobileGL { // for the same reason - writes exactly where the demoted shader reads. Blocks with no // 64-bit member anywhere are never touched. // + // THE MEASURED COST, so the next wave does not re-diagnose it. Four GL 4.3 conformance + // cases fail on BOTH backends and on both an Adreno 830 and a Mali G925 - i.e. on every + // device, because no device has shaderFloat64 and the demotion therefore always runs: + // + // KHR-GL43.shader_storage_buffer_object.basic-stdLayout-case3 + // KHR-GL43.compute_shader.fp64-case1 + // KHR-GL43.compute_shader.fp64-case3 + // ...and the std430 half of the same stdLayout case. + // + // They fail in the two ways this comment predicts and in no other. stdLayout-case3 + // copies a block byte for byte: the output matches the input for bytes [0, 76) and is + // zero from there on, which is exactly the block's size once every double became a + // float and the layout repacked tightly. fp64-case1 reports ceil(2.2) as 2: the + // uniform's double 2.0 is 0x4000000000000000, the demoted read takes its low 32 bits + // (0.0), ceil(0.0 + 0.2) = 1.0f = 0x3F800000 lands in the low half of the 8-byte + // output slot and the whole thing prints as 2. + // + // Fixing them means NOT demoting a double that lives in a buffer block, and carrying + // it as a uvec2 word pair instead - preserving the application's byte layout exactly, + // unpacking to fp32 for arithmetic and repacking on store. That is a large pass with + // the same dmat problem the paragraph above describes (a uvec2 representation cannot + // express a matrix stride either, so it would have to decline dmat types), and the + // default-uniform routing above reflects the demoted module, so a representation + // change there ripples into every glUniform*d. Four of 16085 cases; deliberately not + // attempted. compute_shader.fp64-case2 passes today and any attempt has to keep it + // green. + // // Declines (leaves the module byte-identical, so the caller's existing "this module // still declares Float64" failure path reports it) when the module contains an // operation whose validity depends on the operand really being 64 bits wide: diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.cpp new file mode 100644 index 00000000..359adb23 --- /dev/null +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.cpp @@ -0,0 +1,492 @@ +// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.cpp +// Copyright (c) 2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#include "FlattenAtomicCounterBlockPass.h" + +#include "spirv.hpp" +#include "source/opt/basic_block.h" +#include "source/opt/build_module.h" +#include "source/opt/constants.h" +#include "source/opt/decoration_manager.h" +#include "source/opt/def_use_manager.h" +#include "source/opt/function.h" +#include "source/opt/instruction.h" +#include "source/opt/ir_builder.h" +#include "source/opt/ir_context.h" +#include "source/opt/module.h" +#include "source/opt/type_manager.h" +#include "source/util/make_unique.h" + +#include +#include +#include +#include +#include +#include + +namespace MobileGL { + namespace MG_Util { + namespace ShaderTranspiler { + namespace { + using spvtools::MakeUnique; + using spvtools::opt::BasicBlock; + using spvtools::opt::Function; + using spvtools::opt::Instruction; + using spvtools::opt::InstructionBuilder; + using spvtools::opt::IRContext; + using spvtools::opt::Module; + using spvtools::opt::Operand; + + // Kept in step with MG_Util/ShaderTranspiler/Types.h's + // MAX_ATOMIC_COUNTER_BUFFER_SIZE (16384 bytes), expressed in uint elements. A + // block whose declared byte window is wider than GL will ever let an application + // bind is refused rather than expanded into a huge array. + constexpr uint32_t kMaxCounterElements = 16384u / 4u; + // The lowered block's name always starts with this; the spelling lives in + // Types.h as ATOMIC_COUNTER_BLOCK_PREFIX, which is what the rest of MobileGL + // matches on. Repeated rather than included because that header pulls the whole + // backend-parameter surface into a pass that needs one string. + constexpr const char* kAtomicCounterBlockPrefix = "gl_AtomicCounterBlock"; + // The stride the flattened array is laid out with, and the size of one counter. + constexpr uint32_t kCounterBytes = 4; + + struct MemberPlan { + // Where this member starts, in uint elements from the block's byte 0. + uint32_t elementOffset = 0; + // How many uints it occupies: 1 for a scalar counter, N for `atomic_uint c[N]`. + uint32_t elementCount = 1; + bool isArray = false; + }; + + struct BlockPlan { + Instruction* structType = nullptr; + uint32_t uintTypeId = 0; + std::vector members; + // Access chains rooted at a variable of this block, in the order found. + std::vector chains; + uint32_t totalElements = 0; + }; + + bool NameStartsWithAtomicCounterBlockPrefix(IRContext* context, uint32_t id) { + for (const Instruction& debug : context->module()->debugs2()) { + if (debug.opcode() != spv::Op::OpName || debug.NumInOperands() < 2) continue; + if (debug.GetSingleWordInOperand(0) != id) continue; + const std::string name = debug.GetInOperand(1).AsString(); + return name.compare(0, std::strlen(kAtomicCounterBlockPrefix), + kAtomicCounterBlockPrefix) == 0; + } + return false; + } + + // The literal of the first OpMemberDecorate , or none. + bool TryGetMemberDecorationLiteral(IRContext* context, uint32_t structId, uint32_t member, + spv::Decoration kind, uint32_t* literal) { + for (Instruction* decoration : + context->get_decoration_mgr()->GetDecorationsFor(structId, false)) { + if (decoration->opcode() != spv::Op::OpMemberDecorate || + decoration->NumInOperands() < 4 || + decoration->GetSingleWordInOperand(1) != member || + static_cast(decoration->GetSingleWordInOperand(2)) != kind) { + continue; + } + *literal = decoration->GetSingleWordInOperand(3); + return true; + } + return false; + } + + bool TryGetDecorationLiteral(IRContext* context, uint32_t id, spv::Decoration kind, + uint32_t* literal) { + for (Instruction* decoration : context->get_decoration_mgr()->GetDecorationsFor(id, false)) { + if (decoration->opcode() != spv::Op::OpDecorate || decoration->NumInOperands() < 3 || + static_cast(decoration->GetSingleWordInOperand(1)) != kind) { + continue; + } + *literal = decoration->GetSingleWordInOperand(2); + return true; + } + return false; + } + + bool IsUint32Type(const Instruction* type) { + return type != nullptr && type->opcode() == spv::Op::OpTypeInt && + type->NumInOperands() >= 2 && type->GetSingleWordInOperand(0) == 32u && + type->GetSingleWordInOperand(1) == 0u; + } + + // The member's shape as this pass needs it, or false when it is one the pass + // cannot re-index. + bool DescribeMember(IRContext* context, uint32_t memberTypeId, uint32_t* uintTypeId, + MemberPlan* plan) { + auto* defUseMgr = context->get_def_use_mgr(); + Instruction* memberType = defUseMgr->GetDef(memberTypeId); + if (memberType == nullptr) return false; + + if (IsUint32Type(memberType)) { + plan->isArray = false; + plan->elementCount = 1; + *uintTypeId = memberTypeId; + return true; + } + if (memberType->opcode() != spv::Op::OpTypeArray || memberType->NumInOperands() < 2) { + return false; + } + const uint32_t elementTypeId = memberType->GetSingleWordInOperand(0); + if (!IsUint32Type(defUseMgr->GetDef(elementTypeId))) return false; + // The array's stride must be the tight 4 for the flattening to keep every + // counter on the byte it was declared at. + uint32_t stride = 0; + if (!TryGetDecorationLiteral(context, memberTypeId, spv::Decoration::ArrayStride, &stride) || + stride != kCounterBytes) { + return false; + } + const spvtools::opt::analysis::Constant* length = + context->get_constant_mgr()->FindDeclaredConstant(memberType->GetSingleWordInOperand(1)); + if (length == nullptr || length->AsIntConstant() == nullptr) return false; + const uint32_t count = length->AsIntConstant()->GetU32BitValue(); + if (count == 0u) return false; + plan->isArray = true; + plan->elementCount = count; + *uintTypeId = elementTypeId; + return true; + } + + // Whether the members' offsets already ARE the natural std430 packing, i.e. + // whether the block transpiles as it stands and this pass must leave it alone. + bool IsNaturallyPacked(const std::vector& members) { + uint32_t natural = 0; + for (const MemberPlan& member : members) { + if (member.elementOffset != natural) return false; + natural += member.elementCount; + } + return true; + } + + // Plans every atomic-counter block the module declares that is NOT already + // naturally packed and that this pass can re-index exactly. Reads the module; + // never rewrites it, so the same walk serves both the detection probe and phase 1 + // of the rewrite. + std::vector BuildPlans(IRContext* context) { + std::vector plans; + auto* defUseMgr = context->get_def_use_mgr(); + + std::unordered_map candidateStructs; + for (Instruction& inst : context->module()->types_values()) { + if (inst.opcode() != spv::Op::OpTypeStruct || inst.NumInOperands() == 0) continue; + if (!NameStartsWithAtomicCounterBlockPrefix(context, inst.result_id())) continue; + candidateStructs.emplace(inst.result_id(), &inst); + } + if (candidateStructs.empty()) return plans; + + std::unordered_map variableToStruct; + for (Instruction& inst : context->module()->types_values()) { + if (inst.opcode() != spv::Op::OpVariable) continue; + Instruction* pointerType = defUseMgr->GetDef(inst.type_id()); + if (pointerType == nullptr || pointerType->opcode() != spv::Op::OpTypePointer) continue; + if (candidateStructs.count(pointerType->GetSingleWordInOperand(1)) == 0) continue; + variableToStruct.emplace(inst.result_id(), pointerType->GetSingleWordInOperand(1)); + } + if (variableToStruct.empty()) return plans; + + // A block whose variable is used as anything but an access-chain base (loaded + // whole, handed to a function) cannot be re-indexed; a partially re-indexed + // block would address the wrong counters, so the whole block is refused. + std::unordered_map> chainsByStruct; + std::unordered_set undoableStructs; + for (const auto& [variableId, structId] : variableToStruct) { + defUseMgr->ForEachUser(defUseMgr->GetDef(variableId), [&](Instruction* user) { + switch (user->opcode()) { + case spv::Op::OpName: + case spv::Op::OpDecorate: + case spv::Op::OpDecorateId: + case spv::Op::OpEntryPoint: + return; + case spv::Op::OpAccessChain: + case spv::Op::OpInBoundsAccessChain: + if (user->NumInOperands() >= 2 && + user->GetSingleWordInOperand(0) == variableId) { + chainsByStruct[structId].push_back(user); + return; + } + undoableStructs.insert(structId); + return; + default: + undoableStructs.insert(structId); + return; + } + }); + } + + for (const auto& [structId, structType] : candidateStructs) { + if (undoableStructs.count(structId) != 0) continue; + + BlockPlan plan; + plan.structType = structType; + const uint32_t memberCount = structType->NumInOperands(); + bool expressible = true; + for (uint32_t member = 0; member < memberCount; ++member) { + uint32_t byteOffset = 0; + if (!TryGetMemberDecorationLiteral(context, structId, member, + spv::Decoration::Offset, &byteOffset) || + byteOffset % kCounterBytes != 0u) { + expressible = false; + break; + } + MemberPlan memberPlan; + uint32_t uintTypeId = 0; + if (!DescribeMember(context, structType->GetSingleWordInOperand(member), &uintTypeId, + &memberPlan)) { + expressible = false; + break; + } + if (plan.uintTypeId != 0 && plan.uintTypeId != uintTypeId) { + expressible = false; + break; + } + plan.uintTypeId = uintTypeId; + memberPlan.elementOffset = byteOffset / kCounterBytes; + const uint64_t end = static_cast(memberPlan.elementOffset) + + static_cast(memberPlan.elementCount); + if (end > kMaxCounterElements) { + expressible = false; + break; + } + if (end > plan.totalElements) plan.totalElements = static_cast(end); + plan.members.push_back(memberPlan); + } + if (!expressible || plan.members.empty() || plan.totalElements == 0) continue; + // Already std430: leave it exactly as it is. This is the overwhelmingly + // common answer and the reason the pass can be gated on a cheap probe. + if (IsNaturallyPacked(plan.members)) continue; + + // Every chain must be one of the two shapes the re-index understands: a + // scalar counter reached by (variable, member) or an array element + // reached by (variable, member, index). One that stops at the member, or + // reaches deeper, is not a counter access this pass can move. + const auto chains = chainsByStruct.find(structId); + if (chains != chainsByStruct.end()) { + for (Instruction* chain : chains->second) { + const spvtools::opt::analysis::Constant* memberIndex = + context->get_constant_mgr()->FindDeclaredConstant( + chain->GetSingleWordInOperand(1)); + if (memberIndex == nullptr || memberIndex->AsIntConstant() == nullptr) { + expressible = false; + break; + } + const uint32_t member = memberIndex->AsIntConstant()->GetU32BitValue(); + if (member >= plan.members.size() || + chain->NumInOperands() != (plan.members[member].isArray ? 3u : 2u)) { + expressible = false; + break; + } + plan.chains.push_back(chain); + } + } + if (!expressible) continue; + + plans.push_back(std::move(plan)); + } + return plans; + } + + // The id of |value| as a constant of the same integer type as |likeId|. + uint32_t ConstantLike(IRContext* context, uint32_t likeId, uint32_t value) { + Instruction* likeDef = context->get_def_use_mgr()->GetDef(likeId); + const spvtools::opt::analysis::Type* type = + context->get_type_mgr()->GetType(likeDef->type_id()); + const spvtools::opt::analysis::Constant* constant = + context->get_constant_mgr()->GetConstant(type, {value}); + return context->get_constant_mgr()->GetDefiningInstruction(constant)->result_id(); + } + + Module::inst_iterator PositionOf(IRContext* context, const Instruction* target) { + for (auto it = context->types_values_begin(); it != context->types_values_end(); ++it) { + if (&*it == target) return it; + } + return context->types_values_end(); + } + + // Whether |firstId| is declared before |secondId| in the types/constants section. + bool DeclaredBefore(IRContext* context, uint32_t firstId, uint32_t secondId) { + for (const Instruction& inst : context->module()->types_values()) { + if (inst.result_id() == firstId) return true; + if (inst.result_id() == secondId) return false; + } + return false; + } + + // A fresh `uint[length]` with ArrayStride 4, spliced in immediately BEFORE the + // block that will name it - SPIR-V has no forward references between types, so + // appending it at the end of the section would make the module invalid. A + // duplicate OpTypeArray is legal (SPIR-V 2.8 exempts aggregates from the + // uniqueness rule, and so does spirv-val), so no search for an existing one is + // needed; the LENGTH CONSTANT is not exempt, and if the module already declares + // it after the block there is nowhere legal to put the array - the block is then + // declined and keeps today's behaviour. Returns 0 for that. + uint32_t CreateCounterArrayTypeBefore(IRContext* context, Instruction* structType, + uint32_t uintTypeId, uint32_t length) { + auto* constantMgr = context->get_constant_mgr(); + const spvtools::opt::analysis::Type* uintType = context->get_type_mgr()->GetType(uintTypeId); + if (uintType == nullptr) return 0; + const spvtools::opt::analysis::Constant* lengthConstant = + constantMgr->GetConstant(uintType, {length}); + if (lengthConstant == nullptr) return 0; + + Module::inst_iterator position = PositionOf(context, structType); + if (position == context->types_values_end()) return 0; + Instruction* lengthInst = constantMgr->GetDefiningInstruction(lengthConstant, 0, &position); + if (lengthInst == nullptr) return 0; + if (!DeclaredBefore(context, lengthInst->result_id(), structType->result_id())) return 0; + + const uint32_t arrayTypeId = context->TakeNextId(); + if (arrayTypeId == 0) return 0; + auto arrayType = MakeUnique( + context, spv::Op::OpTypeArray, 0, arrayTypeId, + std::initializer_list{{SPV_OPERAND_TYPE_ID, {uintTypeId}}, + {SPV_OPERAND_TYPE_ID, {lengthInst->result_id()}}}); + Instruction* inserted = structType->InsertBefore(std::move(arrayType)); + context->AnalyzeDefUse(inserted); + context->get_decoration_mgr()->AddDecorationVal( + arrayTypeId, static_cast(spv::Decoration::ArrayStride), kCounterBytes); + return arrayTypeId; + } + + // Drops the annotations the collapsed struct no longer has a member for: every + // OpMemberDecorate and OpMemberName past member 0, plus member 0's own Offset + // (the caller re-adds it as 0). Member 0's OTHER decorations - Coherent, + // Volatile, Restrict and the like, which describe how the counters are accessed + // rather than where they sit - are deliberately kept. + void StripMemberAnnotations(IRContext* context, uint32_t structId) { + std::vector doomed; + for (Instruction* decoration : + context->get_decoration_mgr()->GetDecorationsFor(structId, false)) { + if (decoration->opcode() != spv::Op::OpMemberDecorate || + decoration->NumInOperands() < 3) { + continue; + } + const bool pastMemberZero = decoration->GetSingleWordInOperand(1) != 0u; + const bool isOffset = + static_cast(decoration->GetSingleWordInOperand(2)) == + spv::Decoration::Offset; + if (pastMemberZero || isOffset) doomed.push_back(decoration); + } + for (Instruction& debug : context->module()->debugs2()) { + if (debug.opcode() != spv::Op::OpMemberName || debug.NumInOperands() < 2) continue; + if (debug.GetSingleWordInOperand(0) != structId) continue; + if (debug.GetSingleWordInOperand(1) == 0u) continue; // member 0 keeps its name + doomed.push_back(&debug); + } + for (Instruction* inst : doomed) context->KillInst(inst); + } + } // namespace + + bool FlattenAtomicCounterBlockPass::BinaryHasOffsetAtomicCounterBlock(const Vector& binary) { + if (binary.empty()) { + return false; + } + std::unique_ptr context = spvtools::BuildModule( + SPV_ENV_VULKAN_1_1, + [](spv_message_level_t, const char*, const spv_position_t&, const char*) {}, + binary.data(), binary.size()); + if (!context) { + // Unparseable here means unusable downstream too; let the ordinary transpile + // path produce the error rather than inventing a verdict from it. + return false; + } + return !BuildPlans(context.get()).empty(); + } + + spvtools::opt::Pass::Status FlattenAtomicCounterBlockPass::Process() { + auto* irContext = context(); + const std::vector plans = BuildPlans(irContext); + if (plans.empty()) { + return Status::SuccessWithoutChange; + } + + bool modified = false; + for (const BlockPlan& plan : plans) { + const uint32_t structId = plan.structType->result_id(); + const uint32_t arrayTypeId = CreateCounterArrayTypeBefore( + irContext, plan.structType, plan.uintTypeId, plan.totalElements); + if (arrayTypeId == 0) { + MGLOG_D("[spirv] atomic-counter block %%%u: no legal place for the flattened array " + "type; leaving the block alone", + structId); + continue; + } + + // Re-index BEFORE the struct is collapsed, so the member index each chain + // carries still names the member the plan was built from. + for (Instruction* chain : plan.chains) { + const uint32_t memberIndexId = chain->GetSingleWordInOperand(1); + const uint32_t member = irContext->get_constant_mgr() + ->FindDeclaredConstant(memberIndexId) + ->AsIntConstant() + ->GetU32BitValue(); + const MemberPlan& memberPlan = plan.members[member]; + + std::vector operands; + operands.push_back(chain->GetInOperand(0)); + operands.push_back({SPV_OPERAND_TYPE_ID, {ConstantLike(irContext, memberIndexId, 0u)}}); + if (!memberPlan.isArray) { + operands.push_back( + {SPV_OPERAND_TYPE_ID, + {ConstantLike(irContext, memberIndexId, memberPlan.elementOffset)}}); + } else { + const uint32_t elementId = chain->GetSingleWordInOperand(2); + uint32_t shiftedId = elementId; + if (memberPlan.elementOffset != 0u) { + const spvtools::opt::analysis::Constant* elementConstant = + irContext->get_constant_mgr()->FindDeclaredConstant(elementId); + if (elementConstant != nullptr && elementConstant->AsIntConstant() != nullptr) { + shiftedId = ConstantLike(irContext, elementId, + elementConstant->AsIntConstant()->GetU32BitValue() + + memberPlan.elementOffset); + } else { + InstructionBuilder builder( + irContext, chain, + IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping); + Instruction* elementDef = irContext->get_def_use_mgr()->GetDef(elementId); + shiftedId = builder + .AddBinaryOp(elementDef->type_id(), spv::Op::OpIAdd, + elementId, + ConstantLike(irContext, elementId, + memberPlan.elementOffset)) + ->result_id(); + } + } + operands.push_back({SPV_OPERAND_TYPE_ID, {shiftedId}}); + } + chain->SetInOperands(std::move(operands)); + irContext->UpdateDefUse(chain); + } + + StripMemberAnnotations(irContext, structId); + plan.structType->SetInOperands({{SPV_OPERAND_TYPE_ID, {arrayTypeId}}}); + irContext->UpdateDefUse(plan.structType); + irContext->get_decoration_mgr()->AddMemberDecoration( + structId, 0u, static_cast(spv::Decoration::Offset), 0u); + modified = true; + MGLOG_D("[spirv] atomic-counter block %%%u: collapsed %zu offset member(s) into one " + "%u-element array so std430 can express it", + structId, plan.members.size(), plan.totalElements); + } + + if (!modified) { + return Status::SuccessWithoutChange; + } + irContext->InvalidateAnalysesExceptFor(IRContext::kAnalysisNone); + return Status::SuccessWithChange; + } + + spvtools::Optimizer::PassToken FlattenAtomicCounterBlockPass::CreateFlattenAtomicCounterBlockPass() { + return spvtools::Optimizer::PassToken(MakeUnique()); + } + } // namespace ShaderTranspiler + } // namespace MG_Util +} // namespace MobileGL diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.h new file mode 100644 index 00000000..effb3782 --- /dev/null +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.h @@ -0,0 +1,81 @@ +// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.h +// Copyright (c) 2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#pragma once + +#include "spirv-tools/optimizer.hpp" +#include "source/opt/pass.h" + +#include +#include + +namespace MobileGL { + namespace MG_Util { + namespace ShaderTranspiler { + // glslang's relaxed parse lowers every atomic_uint onto a synthesized storage block + // named gl_AtomicCounterBlock_, and it PRESERVES the application's + // `layout(offset = N)` as the member's SPIR-V Offset decoration. A block whose first + // member sits at offset 8 is not expressible in std140 or std430 - both put member 0 + // at offset 0 - and GLSL ES has no member layout(offset=), so SPIRV-Cross refuses the + // whole stage rather than emit something wrong: + // + // Push constant block cannot be expressed as neither std430 nor std140. + // ES-targets do not support GL_ARB_enhanced_layouts. + // + // (The message says "push constant"; the variable is StorageClass Uniform. Do not + // chase push constants.) The stage never reaches the driver, the program links short + // of it with an EMPTY driver info log, and the dispatch no-ops while the frontend + // still reports the link status glslang published - KHR-GL43.compute_shader.resources + // -atomic-counter's non-zero-offset sibling, and the shape every conformance case + // that declares `layout(binding = B, offset = N)` takes. + // + // The repair is to make the offsets DISAPPEAR rather than to move the buffer. Each + // atomic-counter block is collapsed into ONE `uint` array covering the same byte + // window, member 0 at offset 0 with ArrayStride 4 - a layout std430 expresses + // exactly - and every access is re-indexed to the element that used to be at its + // byte offset. `counters[k]` declared at offset 8 becomes element (2 + k) of the + // array, i.e. byte 8 + 4k, which is the byte the application's counter buffer really + // holds. + // + // Why not simply rebase the offsets to zero and bind the buffer 8 bytes in: because + // glBindBufferRange's offset must be a multiple of + // GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, which the target device reports as 32. + // A byte offset of 8 cannot be expressed as a binding at all, so the correction has + // to live in the shader's indexing, where it costs nothing. + // + // A block that is ALREADY laid out naturally - which is every shader that omits the + // offset qualifier, and so very nearly all of them - is left byte-identical: the + // detection below is the gate, and KHR-GL43.compute_shader.resource-atomic-counter + // (offset 0) is the latch that the no-op case stays a no-op. + // + // Declines the whole block, leaving it untouched, on any shape it cannot re-index + // exactly: a member that is not `uint` or an array of `uint` with stride 4, an offset + // that is not a multiple of 4, a byte window past what GL_MAX_ATOMIC_COUNTER_BUFFER + // _SIZE allows, a member with no Offset decoration at all, or an access chain that + // stops at the member (a pointer handed to a function) rather than reaching a + // counter. + // + // DirectGLES transpile path only. DirectVulkan takes the block's declared offsets + // natively through an explicitly-laid-out descriptor and must see them unchanged. + class FlattenAtomicCounterBlockPass final : public spvtools::opt::Pass { + public: + const char* name() const override { return "mobilegl-flatten-atomic-counter-block"; } + Status Process() override; + + // The detection half, on a serialized module: true when the module declares an + // atomic-counter block whose member offsets are not already the natural std430 + // packing, i.e. whether this pass could change anything. One BuildModule, no + // serialization, so the ~every shader that declares no counter (or declares one + // at offset 0) pays no optimizer round trip. + static bool BinaryHasOffsetAtomicCounterBlock(const Vector& binary); + + static spvtools::Optimizer::PassToken CreateFlattenAtomicCounterBlockPass(); + }; + } // namespace ShaderTranspiler + } // namespace MG_Util +} // namespace MobileGL diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.cpp new file mode 100644 index 00000000..afa25909 --- /dev/null +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.cpp @@ -0,0 +1,610 @@ +// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#include "LegalizeStorageBlockArrayIndexPass.h" + +#include "spirv.hpp" +#include "source/opt/basic_block.h" +#include "source/opt/build_module.h" +#include "source/opt/constants.h" +#include "source/opt/decoration_manager.h" +#include "source/opt/def_use_manager.h" +#include "source/opt/function.h" +#include "source/opt/instruction.h" +#include "source/opt/ir_builder.h" +#include "source/opt/ir_context.h" +#include "source/opt/loop_descriptor.h" +#include "source/opt/module.h" +#include "source/opt/type_manager.h" +#include "source/util/make_unique.h" + +#include +#include +#include + +namespace MobileGL { + namespace MG_Util { + namespace ShaderTranspiler { + namespace { + using spvtools::MakeUnique; + using spvtools::opt::BasicBlock; + using spvtools::opt::Function; + using spvtools::opt::Instruction; + using spvtools::opt::InstructionBuilder; + using spvtools::opt::IRContext; + using spvtools::opt::Operand; + + // GL_MAX_*_SHADER_STORAGE_BLOCKS is 16 on the devices MobileGL targets, and + // each lowered element costs one basic block per write, so a module claiming + // more than this is refused rather than exploded. The largest array in the + // conformance suite is 8. + constexpr uint32_t kMaxLoweredArrayLength = 32; + // One CFG-changing rewrite per round (analyses are dropped after each), so + // the round budget bounds the work on a pathological module. + constexpr int kMaxLoweringRounds = 256; + // Full unrolling copies the body once per iteration, and nothing in the stock + // unroller bounds that. Past this count the loop is left alone and the switch + // lowering, whose cost is the array length rather than the trip count, takes + // it instead. A loop over an array of storage blocks iterates at most + // GL_MAX_*_SHADER_STORAGE_BLOCKS times in any shader that is not already + // broken. + constexpr size_t kMaxUnrolledIterations = 64; + + struct DynamicIndexUse { + Instruction* accessChain = nullptr; + uint32_t arrayLength = 0; + }; + + bool HasDecoration(IRContext* context, uint32_t id, spv::Decoration kind) { + for (Instruction* decoration : context->get_decoration_mgr()->GetDecorationsFor(id, false)) { + if (decoration->opcode() != spv::Op::OpDecorate || + decoration->NumInOperands() < 2) { + continue; + } + if (static_cast(decoration->GetSingleWordInOperand(1)) == kind) { + return true; + } + } + return false; + } + + // Every variable that is an ARRAY OF STORAGE BLOCKS, mapped to that array's + // length. Two spellings are accepted because both reach here depending on the + // SPIR-V version glslang targets: StorageBuffer + Block (1.3, what MobileGL + // asks for) and Uniform + BufferBlock (the pre-1.3 encoding). A UNIFORM block + // array - Uniform + Block - is deliberately NOT collected; see the header. + // + // A length that is not a plain OpConstant (a spec constant) maps to 0: still + // detected as illegal ESSL, never lowered. + std::unordered_map CollectStorageBlockArrays(IRContext* context) { + std::unordered_map blockArrays; + auto* defUseMgr = context->get_def_use_mgr(); + auto* constantMgr = context->get_constant_mgr(); + + for (Instruction& inst : context->module()->types_values()) { + if (inst.opcode() != spv::Op::OpVariable) { + continue; + } + const auto storageClass = + static_cast(inst.GetSingleWordInOperand(0)); + if (storageClass != spv::StorageClass::StorageBuffer && + storageClass != spv::StorageClass::Uniform) { + continue; + } + + Instruction* pointerType = defUseMgr->GetDef(inst.type_id()); + if (pointerType == nullptr || pointerType->opcode() != spv::Op::OpTypePointer) { + continue; + } + Instruction* pointeeType = defUseMgr->GetDef(pointerType->GetSingleWordInOperand(1)); + if (pointeeType == nullptr || pointeeType->opcode() != spv::Op::OpTypeArray) { + continue; + } + Instruction* elementType = defUseMgr->GetDef(pointeeType->GetSingleWordInOperand(0)); + if (elementType == nullptr || elementType->opcode() != spv::Op::OpTypeStruct) { + continue; + } + + const bool isStorageBlock = + storageClass == spv::StorageClass::StorageBuffer + ? HasDecoration(context, elementType->result_id(), spv::Decoration::Block) + : HasDecoration(context, elementType->result_id(), + spv::Decoration::BufferBlock); + if (!isStorageBlock) { + continue; + } + + uint32_t arrayLength = 0; + const spvtools::opt::analysis::Constant* lengthConstant = + constantMgr->FindDeclaredConstant(pointeeType->GetSingleWordInOperand(1)); + if (lengthConstant != nullptr && lengthConstant->AsIntConstant() != nullptr) { + arrayLength = lengthConstant->AsIntConstant()->GetU32BitValue(); + } + blockArrays.emplace(inst.result_id(), arrayLength); + } + return blockArrays; + } + + // "Constant integral expression" in the ESSL sense: an OpConstant (or the + // zero an OpConstantNull stands for). A spec constant is deliberately NOT + // one - SPIRV-Cross prints it as an identifier, which is exactly what the + // driver rejects. + bool IsConstantIndex(IRContext* context, uint32_t indexId) { + Instruction* def = context->get_def_use_mgr()->GetDef(indexId); + return def != nullptr && (def->opcode() == spv::Op::OpConstant || + def->opcode() == spv::Op::OpConstantNull); + } + + // Access chains that index an array of storage blocks with a non-constant. + // Only the FIRST index is considered: it is the one that selects the block, + // and it is the only one ESSL constrains here. Indices inside the block - the + // member selector and any array subscript below it - are legal however they + // are computed, and chains rooted at another access chain are already inside + // one element. + std::vector CollectDynamicIndexUses(IRContext* context) { + std::vector uses; + const std::unordered_map blockArrays = + CollectStorageBlockArrays(context); + if (blockArrays.empty()) { + return uses; + } + + for (Function& function : *context->module()) { + for (BasicBlock& block : function) { + for (Instruction& inst : block) { + if (inst.opcode() != spv::Op::OpAccessChain && + inst.opcode() != spv::Op::OpInBoundsAccessChain) { + continue; + } + if (inst.NumInOperands() < 2) { + continue; + } + const auto arrayIt = blockArrays.find(inst.GetSingleWordInOperand(0)); + if (arrayIt == blockArrays.end()) { + continue; + } + if (IsConstantIndex(context, inst.GetSingleWordInOperand(1))) { + continue; + } + uses.push_back({&inst, arrayIt->second}); + } + } + } + return uses; + } + + // The block index operand of |accessChain| replaced by the constant |element|, + // built at the builder's insertion point. Every later index is copied through + // unchanged: `arr[idx].data[j]` keeps its (legal) dynamic member subscript. + Instruction* CloneChainWithConstantIndex(InstructionBuilder& builder, IRContext* context, + Instruction* accessChain, uint32_t constantIndexId) { + std::vector operands; + operands.reserve(accessChain->NumInOperands()); + for (uint32_t i = 0; i < accessChain->NumInOperands(); ++i) { + if (i == 1) { + operands.push_back({SPV_OPERAND_TYPE_ID, {constantIndexId}}); + } else { + operands.push_back(accessChain->GetInOperand(i)); + } + } + return builder.AddInstruction(MakeUnique(context, accessChain->opcode(), + accessChain->type_id(), + context->TakeNextId(), operands)); + } + + // The id of |element| as a constant of the same integer type as |indexId|. + uint32_t ConstantLikeIndex(IRContext* context, uint32_t indexId, uint32_t element) { + Instruction* indexDef = context->get_def_use_mgr()->GetDef(indexId); + const spvtools::opt::analysis::Type* indexType = + context->get_type_mgr()->GetType(indexDef->type_id()); + const spvtools::opt::analysis::Constant* constant = + context->get_constant_mgr()->GetConstant(indexType, {element}); + return context->get_constant_mgr()->GetDefiningInstruction(constant)->result_id(); + } + + // A 32-bit integer is the only index this pass lowers: OpSwitch matches its + // literals against the selector's width, and every ESSL block-array index is + // an int or uint. + bool IsLowerableIndexType(IRContext* context, uint32_t indexId) { + Instruction* indexDef = context->get_def_use_mgr()->GetDef(indexId); + if (indexDef == nullptr) { + return false; + } + const spvtools::opt::analysis::Type* type = + context->get_type_mgr()->GetType(indexDef->type_id()); + const spvtools::opt::analysis::Integer* integer = + type != nullptr ? type->AsInteger() : nullptr; + return integer != nullptr && integer->width() == 32; + } + + // The condition type OpSelect needs for |resultTypeId|. Before SPIR-V 1.4 a + // scalar bool may not select between vectors, so a vector result needs a bool + // vector of the same width - built by broadcasting the scalar comparison. + // Anything that is neither scalar nor vector (a matrix or struct element) is + // refused: pre-1.4 OpSelect cannot express it either. + bool TryGetSelectConditionType(IRContext* context, uint32_t resultTypeId, + uint32_t* conditionTypeId, uint32_t* dimension) { + auto* typeMgr = context->get_type_mgr(); + const spvtools::opt::analysis::Type* resultType = typeMgr->GetType(resultTypeId); + if (resultType == nullptr) { + return false; + } + + spvtools::opt::analysis::Bool boolType; + if (resultType->AsVector() != nullptr) { + const uint32_t count = resultType->AsVector()->element_count(); + spvtools::opt::analysis::Vector boolVector(&boolType, count); + *conditionTypeId = typeMgr->GetTypeInstruction(&boolVector); + *dimension = count; + return *conditionTypeId != 0; + } + if (resultType->AsInteger() != nullptr || resultType->AsFloat() != nullptr || + resultType->AsBool() != nullptr) { + *conditionTypeId = typeMgr->GetTypeInstruction(&boolType); + *dimension = 1; + return *conditionTypeId != 0; + } + return false; + } + + // Whether fully unrolling |loop| is bounded work. The trip count is read the + // same way the stock unroller reads it, so a loop this declines to measure is + // one CanPerformUnroll would refuse anyway - the hint would be inert on it, + // and the fallback lowering is what handles it. Requires the induction + // variable to already be an OpPhi, which is why this runs after ssa-rewrite. + bool IsBoundedUnrollCandidate(spvtools::opt::Loop* loop) { + const spvtools::opt::BasicBlock* condition = loop->FindConditionBlock(); + if (condition == nullptr) { + return false; + } + const Instruction* induction = loop->FindConditionVariable(condition); + if (induction == nullptr || induction->opcode() != spv::Op::OpPhi) { + return false; + } + size_t iterations = 0; + if (!loop->FindNumberOfIterations(induction, &*condition->ctail(), &iterations)) { + return false; + } + return iterations <= kMaxUnrolledIterations; + } + } // namespace + + bool LegalizeStorageBlockArrayIndexPass::BinaryHasDynamicStorageBlockArrayIndexing( + const std::vector& binary) { + if (binary.empty()) { + return false; + } + std::unique_ptr context = spvtools::BuildModule( + SPV_ENV_VULKAN_1_1, + [](spv_message_level_t, const char*, const spv_position_t&, const char*) {}, + binary.data(), binary.size()); + if (!context) { + return false; + } + return !CollectDynamicIndexUses(context.get()).empty(); + } + + spvtools::opt::Pass::Status LegalizeStorageBlockArrayIndexPass::Process() { + return m_mode == Mode::MarkLoopsForUnroll ? MarkLoopsForUnroll() : LowerToConstantSwitch(); + } + + spvtools::opt::Pass::Status LegalizeStorageBlockArrayIndexPass::MarkLoopsForUnroll() { + auto* irContext = context(); + const std::vector uses = CollectDynamicIndexUses(irContext); + if (uses.empty()) { + return Status::SuccessWithoutChange; + } + + bool modified = false; + for (const DynamicIndexUse& use : uses) { + BasicBlock* block = irContext->get_instr_block(use.accessChain); + if (block == nullptr) { + continue; + } + Function* function = block->GetParent(); + if (function == nullptr) { + continue; + } + + spvtools::opt::LoopDescriptor* loops = irContext->GetLoopDescriptor(function); + for (spvtools::opt::Loop* loop = (*loops)[block->id()]; loop != nullptr; + loop = loop->GetParent()) { + if (!IsBoundedUnrollCandidate(loop)) { + continue; + } + Instruction* mergeInst = loop->GetHeaderBlock()->GetLoopMergeInst(); + // Only a bare `None` control is promoted, and only when no extra + // literal (PartialCount, PeelCount, ...) follows it: the unroller + // tests the control word for equality with Unroll, so ORing the bit + // into a control that already carries something - DontUnroll above + // all - would neither unroll nor mean what it says. + if (mergeInst == nullptr || mergeInst->NumOperands() != 3 || + mergeInst->GetSingleWordOperand(2) != + static_cast(spv::LoopControlMask::MaskNone)) { + continue; + } + mergeInst->SetOperand( + 2, {static_cast(spv::LoopControlMask::Unroll)}); + modified = true; + } + } + + if (!modified) { + return Status::SuccessWithoutChange; + } + MGLOG_D("[spirv] storage-block array index: marked enclosing loops for full unrolling"); + return Status::SuccessWithChange; + } + + spvtools::opt::Pass::Status LegalizeStorageBlockArrayIndexPass::LowerToConstantSwitch() { + auto* irContext = context(); + + bool modified = false; + // Access chains this pass has already refused, so a shape it cannot rewrite + // exactly cannot spin the round loop. + std::unordered_set declined; + + for (int round = 0; round < kMaxLoweringRounds; ++round) { + const std::vector uses = CollectDynamicIndexUses(irContext); + bool progressed = false; + + for (const DynamicIndexUse& use : uses) { + if (declined.count(use.accessChain->result_id()) != 0) { + continue; + } + const LoweringOutcome outcome = LowerOneChain(use.accessChain, use.arrayLength); + if (outcome == LoweringOutcome::Declined) { + declined.insert(use.accessChain->result_id()); + continue; + } + if (outcome == LoweringOutcome::Changed) { + modified = true; + progressed = true; + // A store rewrite splits the block it sat in; every cached + // analysis (and the instruction list this loop is walking) is + // stale from here on. Recollect from scratch. + break; + } + } + + if (!progressed) { + break; + } + } + + if (!modified) { + return Status::SuccessWithoutChange; + } + return Status::SuccessWithChange; + } + + LegalizeStorageBlockArrayIndexPass::LoweringOutcome + LegalizeStorageBlockArrayIndexPass::LowerOneChain(Instruction* accessChain, uint32_t arrayLength) { + auto* irContext = context(); + if (arrayLength == 0 || arrayLength > kMaxLoweredArrayLength) { + MGLOG_D("[spirv] storage-block array index: array length %u is not lowerable", + arrayLength); + return LoweringOutcome::Declined; + } + if (!IsLowerableIndexType(irContext, accessChain->GetSingleWordInOperand(1))) { + return LoweringOutcome::Declined; + } + + std::vector stores; + std::vector loads; + bool unsupportedUse = false; + irContext->get_def_use_mgr()->ForEachUser(accessChain, [&](Instruction* user) { + switch (user->opcode()) { + case spv::Op::OpName: + case spv::Op::OpDecorate: + case spv::Op::OpDecorateId: + return; + case spv::Op::OpStore: + // Only as the pointer. A pointer stored as a *value* is not a storage + // block write and cannot be redirected element-wise. + if (user->GetSingleWordInOperand(0) == accessChain->result_id()) { + stores.push_back(user); + } else { + unsupportedUse = true; + } + return; + case spv::Op::OpLoad: + // Memory operands (Volatile, Aligned, ...) would be dropped by the + // per-element rebuild, so a load carrying any is refused instead. + if (user->NumInOperands() == 1) { + loads.push_back(user); + } else { + unsupportedUse = true; + } + return; + default: + // A pointer passed to a function, copied, chained further, used by an + // atomic, or measured by OpArrayLength cannot be resolved to one + // element here. + unsupportedUse = true; + return; + } + }); + + if (unsupportedUse) { + MGLOG_D("[spirv] storage-block array index: chain %%%u has a use this pass cannot " + "rewrite", + accessChain->result_id()); + return LoweringOutcome::Declined; + } + + if (!loads.empty()) { + return LowerLoad(accessChain, arrayLength, loads.front()); + } + if (!stores.empty()) { + return LowerStore(accessChain, arrayLength, stores.front()); + } + + // No uses left: the chain itself is what detection is still seeing. + irContext->KillInst(accessChain); + irContext->InvalidateAnalysesExceptFor(IRContext::kAnalysisNone); + return LoweringOutcome::Changed; + } + + // switch (idx) { case 0: arr[0]... = v; break; case 1: arr[1]... = v; break; ... } + // + // The block holding the store is split at the store, and the tail becomes the + // switch's merge block, so whatever followed the store still runs exactly once on + // every path. An index outside [0, length) reaches the default target, which is + // the merge block: nothing is stored, which is what indexing a block array out of + // range already meant. + LegalizeStorageBlockArrayIndexPass::LoweringOutcome + LegalizeStorageBlockArrayIndexPass::LowerStore(Instruction* accessChain, uint32_t arrayLength, + Instruction* store) { + auto* irContext = context(); + BasicBlock* block = irContext->get_instr_block(store); + if (block == nullptr) { + return LoweringOutcome::Declined; + } + // Splitting a loop header keeps the label - and so the back edge's target - + // on the first half while the OpLoopMerge moves to the second, which is not + // a loop any more. Refuse instead of producing that. + if (block->GetLoopMergeInst() != nullptr) { + MGLOG_D("[spirv] storage-block array index: store sits in a loop header, declining"); + return LoweringOutcome::Declined; + } + Function* function = block->GetParent(); + if (function == nullptr) { + return LoweringOutcome::Declined; + } + + const uint32_t indexId = accessChain->GetSingleWordInOperand(1); + const uint32_t valueId = store->GetSingleWordInOperand(1); + std::vector memoryOperands; + for (uint32_t i = 2; i < store->NumInOperands(); ++i) { + memoryOperands.push_back(store->GetInOperand(i)); + } + + const uint32_t mergeLabelId = irContext->TakeNextId(); + block->SplitBasicBlock(irContext, mergeLabelId, BasicBlock::iterator(store)); + // |store| now heads the merge block; the per-element stores replace it. + irContext->KillInst(store); + + std::vector> targets; + targets.reserve(arrayLength); + BasicBlock* insertAfter = block; + for (uint32_t element = 0; element < arrayLength; ++element) { + const uint32_t caseLabelId = irContext->TakeNextId(); + auto caseBlock = MakeUnique(MakeUnique( + irContext, spv::Op::OpLabel, 0, caseLabelId, std::initializer_list{})); + caseBlock->SetParent(function); + BasicBlock* casePtr = function->InsertBasicBlockAfter(std::move(caseBlock), insertAfter); + // The builders below register what they add, but this label was built by + // hand: without this the OpSwitch would name a target the def-use manager + // has never seen, which a consistency-checking build calls out. + irContext->AnalyzeDefUse(casePtr->GetLabelInst()); + irContext->set_instr_block(casePtr->GetLabelInst(), casePtr); + + InstructionBuilder caseBuilder( + irContext, casePtr, + IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping); + const uint32_t constantId = ConstantLikeIndex(irContext, indexId, element); + Instruction* elementChain = + CloneChainWithConstantIndex(caseBuilder, irContext, accessChain, constantId); + + std::vector storeOperands; + storeOperands.push_back({SPV_OPERAND_TYPE_ID, {elementChain->result_id()}}); + storeOperands.push_back({SPV_OPERAND_TYPE_ID, {valueId}}); + for (const Operand& memoryOperand : memoryOperands) { + storeOperands.push_back(memoryOperand); + } + caseBuilder.AddInstruction( + MakeUnique(irContext, spv::Op::OpStore, 0, 0, storeOperands)); + caseBuilder.AddBranch(mergeLabelId); + + targets.push_back({Operand::OperandData{element}, caseLabelId}); + insertAfter = casePtr; + } + + InstructionBuilder switchBuilder( + irContext, block, IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping); + switchBuilder.AddSwitch(indexId, mergeLabelId, targets, mergeLabelId); + + if (irContext->get_def_use_mgr()->NumUsers(accessChain) == 0) { + irContext->KillInst(accessChain); + } + irContext->InvalidateAnalysesExceptFor(IRContext::kAnalysisNone); + MGLOG_D("[spirv] storage-block array index: lowered a dynamic write to a %u-way switch", + arrayLength); + return LoweringOutcome::Changed; + } + + // A read needs no control flow: load every element through a constant index and + // pick with OpSelect. Reading the elements the shader did not ask for is safe - + // every one of them is a storage block this stage already declares, and an ES + // driver bounds-checks a storage buffer read that lands outside what is bound. + LegalizeStorageBlockArrayIndexPass::LoweringOutcome + LegalizeStorageBlockArrayIndexPass::LowerLoad(Instruction* accessChain, uint32_t arrayLength, + Instruction* load) { + auto* irContext = context(); + uint32_t conditionTypeId = 0; + uint32_t dimension = 0; + if (!TryGetSelectConditionType(irContext, load->type_id(), &conditionTypeId, &dimension)) { + MGLOG_D("[spirv] storage-block array index: element type is not selectable, declining"); + return LoweringOutcome::Declined; + } + const uint32_t boolTypeId = irContext->get_type_mgr()->GetBoolTypeId(); + const uint32_t indexId = accessChain->GetSingleWordInOperand(1); + + InstructionBuilder builder( + irContext, load, IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping); + + uint32_t selectedId = 0; + for (uint32_t element = 0; element < arrayLength; ++element) { + const uint32_t constantId = ConstantLikeIndex(irContext, indexId, element); + Instruction* elementChain = + CloneChainWithConstantIndex(builder, irContext, accessChain, constantId); + Instruction* elementLoad = builder.AddLoad(load->type_id(), elementChain->result_id()); + if (element == 0) { + // Element 0 is the else-arm of the whole ladder, so an out-of-range + // index reads it - an undefined element for an undefined index. + selectedId = elementLoad->result_id(); + continue; + } + + Instruction* isElement = + builder.AddBinaryOp(boolTypeId, spv::Op::OpIEqual, indexId, constantId); + uint32_t conditionId = isElement->result_id(); + if (dimension > 1) { + std::vector components(dimension, conditionId); + conditionId = builder.AddCompositeConstruct(conditionTypeId, components)->result_id(); + } + selectedId = builder + .AddSelect(load->type_id(), conditionId, elementLoad->result_id(), + selectedId) + ->result_id(); + } + + irContext->ReplaceAllUsesWith(load->result_id(), selectedId); + irContext->KillInst(load); + irContext->InvalidateAnalysesExceptFor(IRContext::kAnalysisNone); + MGLOG_D("[spirv] storage-block array index: lowered a dynamic read to %u constant-indexed " + "loads", + arrayLength); + return LoweringOutcome::Changed; + } + + spvtools::Optimizer::PassToken + LegalizeStorageBlockArrayIndexPass::CreateMarkLoopsForUnrollPass() { + return spvtools::Optimizer::PassToken( + MakeUnique(Mode::MarkLoopsForUnroll)); + } + + spvtools::Optimizer::PassToken + LegalizeStorageBlockArrayIndexPass::CreateLowerToConstantSwitchPass() { + return spvtools::Optimizer::PassToken( + MakeUnique(Mode::LowerToConstantSwitch)); + } + } // namespace ShaderTranspiler + } // namespace MG_Util +} // namespace MobileGL diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.h new file mode 100644 index 00000000..390acc15 --- /dev/null +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.h @@ -0,0 +1,120 @@ +// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.h +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#pragma once +#include "source/opt/pass.h" +#include "spirv-tools/optimizer.hpp" + +#include +#include + +namespace MobileGL { + namespace MG_Util { + namespace ShaderTranspiler { + // GL 4.3 lets an ARRAY OF SHADER STORAGE BLOCKS be indexed with any + // dynamically-uniform expression (GL 4.6 core / GLSL 4.30 4.1.9). GLSL ES keeps + // the stricter ES 3.1 rule - the index must be a *constant integral expression* - + // and the Qualcomm ES compiler enforces it to the letter: + // + // '[' : indexing into an SSBO array using a non-constant expression is not + // permitted + // + // glslang keeps the whole array as ONE SPIR-V variable, so SPIRV-Cross prints + // `layout(binding = N, std430) buffer Blk { ... } arr[4];` plus `arr[i]` verbatim + // and the stage never compiles. The backend program then links nothing and every + // draw or dispatch that uses it is a silent no-op, which reads back as "the buffer + // was never written" rather than as an error - the frontend has already published + // GL_LINK_STATUS = TRUE from glslang's own link. + // + // Verified on the device: an Adreno 830 ES probe with no MobileGL in the loop + // rejects the non-constant subscript with AND without GL_EXT_gpu_shader5 (which + // the driver does advertise), and accepts a constant one. So the ES 3.2 + // "dynamically uniform" relaxation is not a way out - every index really has to + // become a compile-time constant. + // + // Two modes, used as two halves of one legalization in + // ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl - the same shape, for + // the same reasons, as LegalizeFragmentOutputIndexPass: + // + // MarkLoopsForUnroll - `for (int i = 0; i < 4; ++i) arr[i].x = ...` is the + // common shape, and full unrolling turns its index into a literal at no cost + // in emitted code. spirv-opt's CreateLoopUnrollPass only touches loops whose + // OpLoopMerge carries the Unroll control, so this mode sets that hint on + // exactly the loops that enclose an offending access chain, and only when + // their trip count is known and small. Must run AFTER ssa-rewrite: both the + // trip-count check and the unroller need the induction variable as an OpPhi. + // + // LowerToConstantSwitch - the fallback for a genuinely dynamic index + // (uniform-sourced, which is what the CTS indirect-addressing and resource-max + // cases use). A write through such a chain becomes an OpSwitch over the + // array's range with one constant-indexed store per case; a read becomes one + // constant-indexed load per element combined with OpSelect. This is what ANGLE + // does for the same ES 3.1 rule. + // + // Storage blocks only. A UNIFORM block array is a different namespace with its own + // (less strictly enforced) rule and no observed failure, so it is deliberately left + // alone rather than lowered on speculation. + // + // DirectGLES transpile path only: the original module is legal for Vulkan, which + // has no such restriction, and DirectVulkan must keep seeing the array as one + // descriptor array. + // + // The pass DECLINES - leaving the module untouched rather than half-transforming + // it - whenever it meets a shape it cannot rewrite exactly: a pointer handed to a + // function or chained further, an atomic or an OpArrayLength through the chain, a + // load carrying memory operands, a spec-constant array length, an index that is + // not a 32-bit integer, or a store sitting in a loop header block (splitting there + // would move the OpLoopMerge away from the back edge's target). + class LegalizeStorageBlockArrayIndexPass final : public spvtools::opt::Pass { + public: + enum class Mode { + MarkLoopsForUnroll, + LowerToConstantSwitch, + }; + + explicit LegalizeStorageBlockArrayIndexPass(Mode mode) : m_mode(mode) {} + + const char* name() const override { + return m_mode == Mode::MarkLoopsForUnroll + ? "mobilegl-mark-storage-block-array-index-loops" + : "mobilegl-lower-storage-block-array-index"; + } + + Status Process() override; + + static spvtools::Optimizer::PassToken CreateMarkLoopsForUnrollPass(); + static spvtools::Optimizer::PassToken CreateLowerToConstantSwitchPass(); + + // The detection half, on a serialized module: true when an array of storage + // blocks is indexed with anything but an OpConstant. Cheap enough to gate the + // whole legalization on (one BuildModule, no serialization) and used again + // after the folding chain to decide whether the fallback has to run at all. + static bool BinaryHasDynamicStorageBlockArrayIndexing(const std::vector& binary); + + private: + enum class LoweringOutcome { + // The shape is not one this pass can rewrite exactly; the module keeps + // the illegal chain rather than a half-transform of it. + Declined, + Changed, + }; + + Status MarkLoopsForUnroll(); + Status LowerToConstantSwitch(); + + LoweringOutcome LowerOneChain(spvtools::opt::Instruction* accessChain, uint32_t arrayLength); + LoweringOutcome LowerStore(spvtools::opt::Instruction* accessChain, uint32_t arrayLength, + spvtools::opt::Instruction* store); + LoweringOutcome LowerLoad(spvtools::opt::Instruction* accessChain, uint32_t arrayLength, + spvtools::opt::Instruction* load); + + Mode m_mode; + }; + } // namespace ShaderTranspiler + } // namespace MG_Util +} // namespace MobileGL diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp index 0f444611..ac8610b9 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp @@ -49,10 +49,22 @@ namespace MobileGL { imageType->GetSingleWordInOperand(kSampledOperand) == 2u; } + // The other half of the 1D storage-image family: not arrayed. SPIRV-Cross emits + // read and write through one of these correctly, and an ATOMIC through one + // incorrectly (see the header), so this predicate only ever decides anything + // together with the atomic probe below. + bool Is1DNonArrayedStorageImageType(const Instruction* imageType) { + return imageType != nullptr && imageType->opcode() == spv::Op::OpTypeImage && + imageType->NumInOperands() > kSampledOperand && + static_cast(imageType->GetSingleWordInOperand(kDimOperand)) == spv::Dim::Dim1D && + imageType->GetSingleWordInOperand(kArrayedOperand) == 0u && + imageType->GetSingleWordInOperand(kSampledOperand) == 2u; + } + // Any Dim1D image, sampled or storage. Used only to decide whether the Image1D // capability is still needed - deliberately wider than the rewrite's own - // predicate, so a module that also holds a non-arrayed 1D image (which this pass - // leaves to SPIRV-Cross) keeps the capability it still requires. + // predicate, so a module that also holds a 1D image this pass left alone keeps the + // capability it still requires. bool IsDim1DImageType(const Instruction* imageType) { return imageType != nullptr && imageType->opcode() == spv::Op::OpTypeImage && imageType->NumInOperands() > kSampledOperand && @@ -110,6 +122,60 @@ namespace MobileGL { return opcode == spv::Op::OpImageQuerySize || opcode == spv::Op::OpImageQuerySizeLod || opcode == spv::Op::OpImageQueryLevels || opcode == spv::Op::OpImageQuerySamples; } + + // Which 1D storage images this module is to be rewritten for. Arrayed ones always; + // non-arrayed ones only when an atomic reaches one, because that is the only shape + // SPIRV-Cross gets wrong for them and taking over a path it gets right would be a + // regression looking for somewhere to happen. + struct LoweringScope { + bool arrayed = false; + bool nonArrayed = false; + + bool Any() const { return arrayed || nonArrayed; } + bool Covers(const Instruction* imageType) const { + return (arrayed && Is1DArrayStorageImageType(imageType)) || + (nonArrayed && Is1DNonArrayedStorageImageType(imageType)); + } + }; + + // OpImageTexelPointer is the operand path of every imageAtomic*; nothing else in a + // GLSL-derived module produces one. + bool PerformsAtomicOnNonArrayed1DImage(IRContext* context) { + for (auto& function : *context->module()) { + for (auto& block : function) { + for (auto& instruction : block) { + if (instruction.opcode() != spv::Op::OpImageTexelPointer || + instruction.NumInOperands() < 1) { + continue; + } + if (Is1DNonArrayedStorageImageType( + ResolveImageType(context, instruction.GetSingleWordInOperand(0)))) { + return true; + } + } + } + } + return false; + } + + // One walk of the type table, then - and only when the module declares a + // non-arrayed 1D storage image at all - one walk of the code. Every other shader + // pays the type walk and nothing else. + LoweringScope ResolveLoweringScope(IRContext* context) { + LoweringScope scope; + bool hasNonArrayed = false; + for (const Instruction& type : context->module()->types_values()) { + if (Is1DArrayStorageImageType(&type)) { + scope.arrayed = true; + } else if (Is1DNonArrayedStorageImageType(&type)) { + hasNonArrayed = true; + } + } + if (hasNonArrayed) { + scope.nonArrayed = PerformsAtomicOnNonArrayed1DImage(context); + } + return scope; + } } // namespace Lower1DArrayImagesPass::ModuleTraits Lower1DArrayImagesPass::InspectBinary(const Vector& binary) { @@ -126,15 +192,11 @@ namespace MobileGL { // The type table settles it for the cheap half, and it is the half almost every // shader takes: no such type declared, nothing to inspect further. - for (const Instruction& type : context->module()->types_values()) { - if (Is1DArrayStorageImageType(&type)) { - traits.declaresImage = true; - break; - } - } - if (!traits.declaresImage) { + const LoweringScope scope = ResolveLoweringScope(context.get()); + if (!scope.Any()) { return traits; } + traits.declaresImage = true; for (auto& function : *context->module()) { for (auto& block : function) { @@ -142,7 +204,7 @@ namespace MobileGL { if (!QueriesImageSize(instruction.opcode()) || instruction.NumInOperands() < 1) { continue; } - if (Is1DArrayStorageImageType( + if (scope.Covers( ResolveImageType(context.get(), instruction.GetSingleWordInOperand(0)))) { traits.queriesImageSize = true; return traits; @@ -160,27 +222,21 @@ namespace MobileGL { // Nothing to do unless the module actually declares one. Every other shader pays // one walk of the type table and is handed back unchanged. - bool hasType = false; - for (const Instruction& type : irContext->types_values()) { - if (Is1DArrayStorageImageType(&type)) { - hasType = true; - break; - } - } - if (!hasType) { + const LoweringScope scope = ResolveLoweringScope(irContext); + if (!scope.Any()) { return Status::SuccessWithoutChange; } // The same refusal the caller makes, restated here so the pass is safe wherever // it is registered. Rewriting the type while leaving an OpImageQuerySize on it // produces a query whose result type has one component too few - an invalid - // module - and there is no correct two-component size to substitute, because the - // ES texture genuinely has a height the GL one does not. + // module - and there is no correct narrower size to substitute, because the ES + // texture genuinely has a height the GL one does not. for (auto& function : *irContext->module()) { for (auto& block : function) { for (auto& instruction : block) { if (QueriesImageSize(instruction.opcode()) && instruction.NumInOperands() >= 1 && - Is1DArrayStorageImageType( + scope.Covers( ResolveImageType(irContext, instruction.GetSingleWordInOperand(0)))) { return Status::SuccessWithoutChange; } @@ -188,9 +244,12 @@ namespace MobileGL { } } - // (u, layer) -> (u, 0, layer). The height the ES 2D array carries is 1, so Y is - // always 0 and the layer has to move from the second component to the third; a - // plain widening that appended the 0 would read layer 0 of every access instead. + // Arrayed: (u, layer) -> (u, 0, layer). The height the ES 2D array carries is 1, + // so Y is always 0 and the layer has to move from the second component to the + // third; a plain widening that appended the 0 would read layer 0 of every access + // instead. Non-arrayed: u -> (u, 0), which is exactly what SPIRV-Cross itself + // writes for the operations it does widen - reproduced here so read, write and + // atomic all come out of one place. for (auto& function : *irContext->module()) { for (auto& block : function) { for (auto& instruction : block) { @@ -199,38 +258,44 @@ namespace MobileGL { instruction.NumInOperands() <= coordinateOperand) { continue; } - if (!Is1DArrayStorageImageType( - ResolveImageType(irContext, instruction.GetSingleWordInOperand(0)))) { + const Instruction* imageType = + ResolveImageType(irContext, instruction.GetSingleWordInOperand(0)); + if (!scope.Covers(imageType)) { continue; } + const bool arrayed = Is1DArrayStorageImageType(imageType); const uint32_t coordinateId = instruction.GetSingleWordInOperand(coordinateOperand); // Built from the COORDINATE's own component type rather than a - // hardcoded signed int. GLSL only ever spells these ivec2, but SPIR-V - // permits an unsigned coordinate, and extracting a uint component - // into an int result is an invalid module rather than a wrong answer - - // the kind of defect that reaches a driver as "compiles here, not - // there". + // hardcoded signed int. GLSL only ever spells these int/ivec2, but + // SPIR-V permits an unsigned coordinate, and extracting a uint + // component into an int result is an invalid module rather than a + // wrong answer - the kind of defect that reaches a driver as "compiles + // here, not there". Instruction* coordinateDef = irContext->get_def_use_mgr()->GetDef(coordinateId); if (coordinateDef == nullptr) return Status::Failure; const auto* coordinateType = typeMgr->GetType(coordinateDef->type_id()); - const auto* coordinateVector = coordinateType != nullptr ? coordinateType->AsVector() - : nullptr; - if (coordinateVector == nullptr || coordinateVector->element_count() != 2) { + if (coordinateType == nullptr) return Status::Failure; + // Arrayed coordinates are the two-component (u, layer); non-arrayed + // ones are the bare scalar u. Anything else is a shape this pass does + // not translate, and declining leaves the module byte for byte. + const auto* coordinateVector = arrayed ? coordinateType->AsVector() : nullptr; + if (arrayed && (coordinateVector == nullptr || coordinateVector->element_count() != 2)) { return Status::Failure; } - const auto* component = coordinateVector->element_type(); + const auto* component = + arrayed ? coordinateVector->element_type() : coordinateType; const auto* componentInteger = component != nullptr ? component->AsInteger() : nullptr; if (componentInteger == nullptr) return Status::Failure; - spvtools::opt::analysis::Vector widenedVector(component, 3); - const uint32_t int3TypeId = typeMgr->GetTypeInstruction(&widenedVector); + spvtools::opt::analysis::Vector widenedVector(component, arrayed ? 3 : 2); + const uint32_t widenedTypeId = typeMgr->GetTypeInstruction(&widenedVector); const uint32_t intTypeId = typeMgr->GetTypeInstruction(component); const uint32_t zeroId = componentInteger->IsSigned() ? constantMgr->GetSIntConstId(0) : constantMgr->GetUIntConstId(0); - if (int3TypeId == 0 || intTypeId == 0 || zeroId == 0) { + if (widenedTypeId == 0 || intTypeId == 0 || zeroId == 0) { return Status::Failure; } @@ -238,15 +303,20 @@ namespace MobileGL { irContext, &instruction, IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping); - Instruction* u = - builder.AddCompositeExtract(intTypeId, coordinateId, {0}); - Instruction* layer = - builder.AddCompositeExtract(intTypeId, coordinateId, {1}); - if (u == nullptr || layer == nullptr) { - return Status::Failure; + Instruction* widened = nullptr; + if (arrayed) { + Instruction* u = + builder.AddCompositeExtract(intTypeId, coordinateId, {0}); + Instruction* layer = + builder.AddCompositeExtract(intTypeId, coordinateId, {1}); + if (u == nullptr || layer == nullptr) { + return Status::Failure; + } + widened = builder.AddCompositeConstruct( + widenedTypeId, {u->result_id(), zeroId, layer->result_id()}); + } else { + widened = builder.AddCompositeConstruct(widenedTypeId, {coordinateId, zeroId}); } - Instruction* widened = builder.AddCompositeConstruct( - int3TypeId, {u->result_id(), zeroId, layer->result_id()}); if (widened == nullptr) { return Status::Failure; } @@ -256,21 +326,22 @@ namespace MobileGL { } } - // Only now, with no access still spelling the 1D-array coordinate, does the type - // become the 2D array one. Arrayed stays 1: this is a 2D ARRAY image, which is - // what the texture was stored as. + // Only now, with no access still spelling a 1D coordinate, does the type become + // the 2D one. Arrayed is left exactly as it was - a 1D array becomes a 2D ARRAY + // image, which is what the texture was stored as, and a non-arrayed 1D becomes the + // plain 2D image MobileGL stores a GL_TEXTURE_1D in (height 1). for (Instruction& type : irContext->types_values()) { - if (Is1DArrayStorageImageType(&type)) { + if (scope.Covers(&type)) { type.SetInOperand(kDimOperand, {static_cast(spv::Dim::Dim2D)}); } } // Image1D describes the types just rewritten - but only drop it if no 1D image - // type is left at all. A module may hold a non-arrayed 1D storage image, which - // this pass deliberately leaves to SPIRV-Cross, and that one still needs the - // capability. Shader is always declared by any module reaching here, so restating - // it keeps the instruction valid without leaving a capability a consumer could - // key off. + // type is left at all. A module may hold a 1D image this pass left alone (a + // SAMPLED one always, and a non-arrayed storage one whenever no atomic reaches + // it), and that one still needs the capability. Shader is always declared by any + // module reaching here, so restating it keeps the instruction valid without + // leaving a capability a consumer could key off. bool anyDim1DLeft = false; for (const Instruction& type : irContext->types_values()) { if (IsDim1DImageType(&type)) { diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h index 10b3f5c6..72b27a2f 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h @@ -46,13 +46,30 @@ namespace MobileGL { // through it has its coordinate widened from (u, layer) to (u, 0, layer). SPIRV-Cross // is then looking at an ordinary 2D array image and its 1D path never fires. // + // The NON-arrayed 1D storage image is handled too, but only in one shape. SPIRV-Cross + // applies the widening above in OpImageRead (spirv_glsl.cpp) and in OpImageWrite - and + // NOT in OpImageTexelPointer, which is the operand path every imageAtomic* goes + // through. So `imageAtomicAdd(g_image_1d, coord.x, 2)` comes out with a SCALAR + // coordinate against a variable it declared `iimage2D`, and the ES compiler answers + // "'imageAtomicAdd' : no matching overloaded function found" - losing the whole stage + // and with it every other image in it, which is how + // KHR-GL4x.shader_image_load_store.basic-allTargets-atomic lost a seven-image fragment + // shader over one of them. + // + // That case is lowered here for the same reason as the arrayed one: the type becomes + // Dim2D and every coordinate is widened from u to (u, 0) in the module, so SPIRV-Cross + // has no 1D image left to emulate and read, write and atomic are all spelled by one + // piece of code. It is gated on the module ACTUALLY performing an image atomic on such + // an image, so a shader that only loads and stores through a 1D image keeps taking + // SPIRV-Cross's own (correct) emission byte for byte and this pass cannot regress it. + // // Deliberately narrow, on three axes: // // * STORAGE images only (Sampled == 2). Sampled images reach SPIRV-Cross's sampler // path, which is correct today; rewriting them would replace working emission // with our own for no reason. - // * ARRAYED only. A non-arrayed 1D storage image is emitted correctly by the same - // SPIRV-Cross code, and is left to it. + // * ARRAYED always; NON-arrayed only when the module holds an OpImageTexelPointer + // into one, i.e. only when SPIRV-Cross's own emission is already broken for it. // * ESSL only. Vulkan has VK_IMAGE_VIEW_TYPE_1D_ARRAY natively and Magma binds it // directly, so the module must reach that backend unchanged. // @@ -81,13 +98,15 @@ namespace MobileGL { // cost one module parse and no optimizer run at all - not one parse to ask about // size queries and a second inside an Optimizer that then early-outs. struct ModuleTraits { - // The module declares a 1D-array storage image, i.e. there is anything to do. + // The module declares an image this pass would rewrite - a 1D-array storage + // image, or a non-arrayed 1D storage image the module performs an atomic on - + // i.e. there is anything to do. bool declaresImage = false; // ...and queries its size, which is the shape this pass refuses to translate: - // afterwards the image is a 2D array, so the query yields three components - // where the shader consumes two, and there is no correct two-component answer - // to substitute. The caller leaves such a module alone rather than half - // rewriting it. + // afterwards the image is a 2D (array) one, so the query yields a component + // more than the shader consumes, and there is no correct narrower answer to + // substitute. The caller leaves such a module alone rather than half rewriting + // it. bool queriesImageSize = false; }; static ModuleTraits InspectBinary(const Vector& binary); diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp index 0cbc0cba..29b32225 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp @@ -48,13 +48,16 @@ namespace MobileGL { return SPVC_BASETYPE_UNKNOWN; } - // Record one flattened leaf uniform of the global UBO into the metadata maps. - static void RecordGlobalUboLeaf(const SpvReflectBlockVariable& member, const String& name, - Uint32 offsetInUBO, SpvcMetadata& metadata) { + // Write one metadata entry. `name` is already the glslang-reflection spelling the + // GL uniform locations are keyed on, and `arrayStride`/`sizeInBytes` describe the + // entry rather than the whole declaration (they differ for a sub-array of an + // array-of-arrays - see RecordGlobalUboLeaf). + static void RecordGlobalUboLeafEntry(const SpvReflectBlockVariable& member, const String& name, + Uint32 offsetInUBO, Uint32 arrayStride, SizeT sizeInBytes, + SpvcMetadata& metadata) { metadata.plainUniformOffsetsInUBO[name] = offsetInUBO; - metadata.plainUniformMemberSizesInBytes[name] = member.size; - metadata.plainUniformArrayStridesInUBO[name] = - member.array.dims_count > 0 ? member.array.stride : 0; + metadata.plainUniformMemberSizesInBytes[name] = sizeInBytes; + metadata.plainUniformArrayStridesInUBO[name] = arrayStride; Uint32 vectorSize = member.numeric.vector.component_count; if (vectorSize == 0) vectorSize = 1; @@ -67,6 +70,70 @@ namespace MobileGL { }; } + // Record one flattened leaf uniform of the global UBO into the metadata maps. + // + // SPIRV-Reflect keeps `float u[2][3]` as ONE leaf carrying every dimension in + // array.dims[] and the INNERMOST element stride in array.stride (the outer + // ArrayStride decoration is overwritten as ParseType recurses into the element + // type, which is also why array.size == product(dims) * stride). glslang's + // reflection - which owns the names GL uniform locations are keyed on - stops at + // "reflection granularity" instead (reflection.cpp: !type.isArrayOfArrays()), so + // the same declaration arrives on the GL side as "u[0]" and "u[1]", each a + // `float[3]` holding its own three locations. + // + // Emitting a single "u" leaf here therefore only ever routes the FIRST sub-array: + // the routing loop stops as soon as a location belongs to a different uniform, and + // every element from "u[1][0]" on finds no offset and falls through to the fallback + // scratch storage at the tail of the shadow - bytes the GPU never reads, so those + // glUniform writes are silently lost + // (KHR-GLES31.explicit_uniform_location.uniform-loc-arrays-of-arrays). Expand every + // dimension but the last, exactly as glslang does, and give each sub-array its own + // byte offset. + static void RecordGlobalUboLeaf(const SpvReflectBlockVariable& member, const String& name, + Uint32 offsetInUBO, SpvcMetadata& metadata) { + const Uint32 arrayStride = member.array.dims_count > 0 ? member.array.stride : 0; + const Bool isArrayOfArrays = member.array.dims_count > 1 && arrayStride > 0; + if (!isArrayOfArrays) { + RecordGlobalUboLeafEntry(member, name, offsetInUBO, arrayStride, member.size, metadata); + return; + } + + // Extent 0 is SPIRV-Reflect's OpTypeRuntimeArray marker, which a plain uniform + // cannot be - but it must not be expanded (or divided by) if it ever appears. + Uint32 subArrayCount = 1; + for (Uint32 dim = 0; dim + 1 < member.array.dims_count; ++dim) { + const Uint32 extent = member.array.dims[dim]; + if (extent == 0) { + MGLOG_W_ONCE("RecordGlobalUboLeaf: multi-dimensional uniform '%s' has a non-constant " + "dimension, recording the base entry only", + name.c_str()); + RecordGlobalUboLeafEntry(member, name, offsetInUBO, arrayStride, member.size, metadata); + return; + } + subArrayCount *= extent; + } + const Uint32 innerExtent = member.array.dims[member.array.dims_count - 1] > 0 + ? member.array.dims[member.array.dims_count - 1] + : 1; + const Uint32 subArrayStride = innerExtent * arrayStride; + + // Row-major odometer over dims[0 .. dims_count-2]: the last dimension varies + // fastest, so `subArray` counts sub-arrays in exactly memory order. + Vector indices(member.array.dims_count - 1, 0); + for (Uint32 subArray = 0; subArray < subArrayCount; ++subArray) { + String elementName = name; + for (const Uint32 index : indices) { + elementName += "[" + std::to_string(index) + "]"; + } + RecordGlobalUboLeafEntry(member, elementName, offsetInUBO + subArray * subArrayStride, + arrayStride, subArrayStride, metadata); + for (SizeT dim = indices.size(); dim-- > 0;) { + if (++indices[dim] < member.array.dims[dim]) break; + indices[dim] = 0; + } + } + } + // Flatten a (possibly nested struct / struct array) member of the global UBO // into leaf entries named the way glslang reflection names plain uniforms: // "s[0].b[1].b" for `uniform S s[2]` with `struct T { vec2 b[2]; }` members. diff --git a/MobileGL/MG_Util/ShaderTranspiler/TranslationCache.h b/MobileGL/MG_Util/ShaderTranspiler/TranslationCache.h index ec2bf6b0..980ddb6b 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/TranslationCache.h +++ b/MobileGL/MG_Util/ShaderTranspiler/TranslationCache.h @@ -564,8 +564,20 @@ namespace MobileGL::MG_Util::ShaderTranspiler { // compile-time constants (GLSL_ES true, VULKAN_SEMANTICS false); // * the SPIR-V validation switch, as in L1. // - // Unconditional passes (StripUboMemberRelaxedPrecision, LowerRectImages, - // Lower1DArrayImages) take no input but the module and so need no key material. + // Unconditional passes take no input but the module and so need no key material: + // StripUboMemberRelaxedPrecision, LowerRectImages, Lower1DArrayImages, + // LegalizeStorageBlockArrayIndexing and FlattenAtomicCounterBlockOffsets. Each self-gates + // on the module's own content and is armed by nothing, so the SPIR-V already in this key + // covers them completely. + // + // THE TEST FOR THAT CLAIM IS NOT THE SIGNATURE. LowerViewportIndexForEssl is equally + // module-only to look at, yet SupportsViewportArray is in this key because that bit ARMS + // it at the call site. So a new pass needs BOTH checks - what it takes, and what decides + // whether it runs - before "no key material" is a conclusion rather than an assumption. + // Note also where an application-authored value can hide: the atomic-counter + // layout(offset = N) qualifiers that FlattenAtomicCounterBlockOffsets rewrites are not a + // separate input at all, because glslang already baked them into the module as member + // Offset decorations - i.e. into the key's biggest field. struct EsslTranslationResult { String essl; // Which interface blocks FlattenXfbInterfaceBlocksForEssl actually rewrote