diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 23a90ab1..8161fe8f 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -3805,6 +3805,34 @@ void main() { imageStore(uni_image, ivec2(0), uvec4(1u)); } EXPECT_NE(DecompileToEssl(baked).find("rgba32ui"), String::npos); } +// Review finding. Every use has to be one the retype can carry end to end, and the decision has +// to be made BEFORE anything is mutated - a half-retyped module is not something a later decline +// could undo. An image handed to a FUNCTION is the shape that reaches SPIRV-Cross intact (nothing +// in the ESSL chain inlines), and its OpFunctionCall is a use this pass does not follow. +TEST_F(ProgramUtilTest, BakeImageFormatsDeclinesAnImagePassedToAFunction) { + using namespace MG_Util::ShaderTranspiler; + + const Vector spirv = BuildSpirvForStage(R"(#version 430 core +layout (local_size_x = 1) in; +writeonly uniform uimage2D uni_image; +void writeIt(writeonly uimage2D img) { imageStore(img, ivec2(0), uvec4(1u)); } +void main() { writeIt(uni_image); } +)", + GL_COMPUTE_SHADER); + ASSERT_FALSE(spirv.empty()); + ASSERT_TRUE(ShaderCompiler::DeclaresFormatlessStorageImage(spirv)); + + SpirvValidationScope validationOn(true); + const Uint64 failuresBefore = ShaderCompiler::SpirvValidationFailureCount(); + + Vector baked; + ASSERT_TRUE(ShaderCompiler::BakeImageFormatsForEssl(spirv, {{"uni_image", kGlR32ui}}, baked)); + EXPECT_EQ(baked, spirv) << "a shape the retype cannot follow must leave the module untouched, " + "not partly rewritten:\n" + << DisassembleSpirv(baked); + EXPECT_EQ(ShaderCompiler::SpirvValidationFailureCount(), failuresBefore); +} + // spirv-val requires the Image Format's component class to agree with the OpTypeImage's Sampled // Type. Binding a uint format to a float image is an application error GL leaves undefined; // baking it would turn that into an INVALID module, which is strictly worse than the compile diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/BakeImageFormatsPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/BakeImageFormatsPass.cpp index 126295b3..aa5bfc51 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/BakeImageFormatsPass.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/BakeImageFormatsPass.cpp @@ -459,9 +459,13 @@ namespace MobileGL { candidate.loads.push_back(user); return; } - // OpImageTexelPointer takes the image POINTER and yields a pointer to the - // sampled type, which the format operand does not enter into. - if (user->opcode() == spv::Op::OpImageTexelPointer) return; + // OpImageTexelPointer is DECLINED, not allowed through. Its result type + // does not depend on the format, but spirv-val requires the image behind + // an atomic to be r32i/r32ui/r32f, so baking any other format here would + // turn a module the validator accepts (Unknown is exempt) into one it + // rejects. GLSL cannot express an atomic on a format-less image anyway - + // the format qualifier is what makes an image atomic legal - so nothing + // reachable is being given up. rewritable = false; }); if (!rewritable) continue; @@ -475,7 +479,6 @@ namespace MobileGL { candidate.loads.push_back(user); return; } - if (user->opcode() == spv::Op::OpImageTexelPointer) return; rewritable = false; }); } @@ -558,8 +561,8 @@ namespace MobileGL { // new operand's definition comes last - the join case is exactly where those two // differ, and putting the clone after the original alone is what left an // OpVariable naming a pointer type declared below it. - auto cloneTypeWithOperand = [&](Instruction* original, uint32_t operandIndex, - uint32_t value) -> uint32_t { + auto cloneTypeWithOperand = [&](Instruction* original, uint32_t operandIndex, uint32_t value, + bool valueIsId) -> uint32_t { const auto cacheKey = std::make_pair(original->result_id(), value); const auto cached = cloneCache.find(cacheKey); if (cached != cloneCache.end()) return cached->second; @@ -580,7 +583,11 @@ namespace MobileGL { return existing; } const uint32_t newId = clone->result_id(); - Instruction* anchor = laterInGlobals(original, defUseMgr->GetDef(value)); + // Only an ID operand names a definition the clone has to sit behind. The + // format operand is a LITERAL, and looking it up would resolve some unrelated + // instruction that happens to carry that number as its result id. + Instruction* anchor = + valueIsId ? laterInGlobals(original, defUseMgr->GetDef(value)) : original; if (anchor == nullptr) return 0; Instruction* inserted = clone.release(); inserted->InsertAfter(anchor); @@ -598,7 +605,8 @@ namespace MobileGL { bool changed = false; for (Candidate& candidate : candidates) { const uint32_t newImageId = cloneTypeWithOperand(candidate.chain.imageType, kImageFormatOperand, - static_cast(candidate.format)); + static_cast(candidate.format), + /*valueIsId=*/false); if (newImageId == 0) return Status::Failure; // The pointer-to-image type every access chain and every single-image @@ -606,11 +614,11 @@ namespace MobileGL { uint32_t newPointeeId = newImageId; if (candidate.chain.arrayType != nullptr) { newPointeeId = cloneTypeWithOperand(candidate.chain.arrayType, kArrayElementOperand, - newImageId); + newImageId, /*valueIsId=*/true); if (newPointeeId == 0) return Status::Failure; } const uint32_t newVariablePointerId = cloneTypeWithOperand( - candidate.chain.pointerType, kPointerPointeeOperand, newPointeeId); + candidate.chain.pointerType, kPointerPointeeOperand, newPointeeId, /*valueIsId=*/true); if (newVariablePointerId == 0) return Status::Failure; candidate.variable->SetResultType(newVariablePointerId); @@ -630,7 +638,7 @@ namespace MobileGL { Instruction* oldResultType = defUseMgr->GetDef(accessChain->type_id()); if (oldResultType == nullptr) return Status::Failure; const uint32_t newResultType = - cloneTypeWithOperand(oldResultType, kPointerPointeeOperand, newImageId); + cloneTypeWithOperand(oldResultType, kPointerPointeeOperand, newImageId, /*valueIsId=*/true); if (newResultType == 0) return Status::Failure; accessChain->SetResultType(newResultType); defUseMgr->AnalyzeInstUse(accessChain);