From 39046169da60396c651b664aa67ea52a04146d2f Mon Sep 17 00:00:00 2001 From: Jamie Madill Date: Mon, 8 Feb 2016 15:05:17 -0500 Subject: [PATCH] CollectVariables: Don't include block name in field name. The spec mandates that the instance name of a block determines how the active uniform name for this field is reported. However, our handling of this was a bit bugged. We would include the proper prefix on the compiler-side, but this mangled the hashing, and was also not strictly needed. We now also expose the instance name, so we can determine the proper prefix for variable linking on the GL-side of things. This also is consistent with how we handle other spec issues, where the GL-side handles the GL-API specific functionality. This also allows us to fix name hashing of instanced uniform blocks, which was previously broken because we would hash the full name of the active uniform, instead of just the field. BUG=angleproject:1306 Change-Id: I06ace6dbc3f75fdd8129677360dcc142aa89136e Reviewed-on: https://chromium-review.googlesource.com/326681 Reviewed-by: Geoff Lang Commit-Queue: Jamie Madill --- include/GLSLANG/ShaderLang.h | 2 +- include/GLSLANG/ShaderVars.h | 5 +- src/compiler/translator/ShaderVars.cpp | 5 + src/compiler/translator/VariableInfo.cpp | 22 +-- src/libANGLE/Program.cpp | 2 +- src/libANGLE/renderer/d3d/ProgramD3D.cpp | 4 +- .../compiler_tests/CollectVariables_test.cpp | 154 ++++++++++++++---- 7 files changed, 135 insertions(+), 59 deletions(-) diff --git a/include/GLSLANG/ShaderLang.h b/include/GLSLANG/ShaderLang.h index 5b57a2863..d02723e76 100644 --- a/include/GLSLANG/ShaderLang.h +++ b/include/GLSLANG/ShaderLang.h @@ -48,7 +48,7 @@ typedef unsigned int GLenum; // Version number for shader translation API. // It is incremented every time the API changes. -#define ANGLE_SH_VERSION 142 +#define ANGLE_SH_VERSION 143 typedef enum { SH_GLES2_SPEC = 0x8B40, diff --git a/include/GLSLANG/ShaderVars.h b/include/GLSLANG/ShaderVars.h index 0e82e7e61..af9b65b7f 100644 --- a/include/GLSLANG/ShaderVars.h +++ b/include/GLSLANG/ShaderVars.h @@ -200,6 +200,9 @@ struct COMPILER_EXPORT InterfaceBlock InterfaceBlock(const InterfaceBlock &other); InterfaceBlock &operator=(const InterfaceBlock &other); + // Fields from blocks with non-empty instance names are prefixed with the block name. + std::string fieldPrefix() const; + std::string name; std::string mappedName; std::string instanceName; @@ -210,6 +213,6 @@ struct COMPILER_EXPORT InterfaceBlock std::vector fields; }; -} +} // namespace sh #endif // GLSLANG_SHADERVARS_H_ diff --git a/src/compiler/translator/ShaderVars.cpp b/src/compiler/translator/ShaderVars.cpp index 7a86af699..8f931b9bd 100644 --- a/src/compiler/translator/ShaderVars.cpp +++ b/src/compiler/translator/ShaderVars.cpp @@ -393,4 +393,9 @@ InterfaceBlock &InterfaceBlock::operator=(const InterfaceBlock &other) return *this; } +std::string InterfaceBlock::fieldPrefix() const +{ + return instanceName.empty() ? "" : name; } + +} // namespace sh diff --git a/src/compiler/translator/VariableInfo.cpp b/src/compiler/translator/VariableInfo.cpp index eb2a2d8d0..3b6aa6a68 100644 --- a/src/compiler/translator/VariableInfo.cpp +++ b/src/compiler/translator/VariableInfo.cpp @@ -16,18 +16,6 @@ namespace sh namespace { -TString InterfaceBlockFieldName(const TInterfaceBlock &interfaceBlock, const TField &field) -{ - if (interfaceBlock.hasInstanceName()) - { - return interfaceBlock.name() + "." + field.name(); - } - else - { - return field.name(); - } -} - BlockLayoutType GetBlockLayoutType(TLayoutBlockStorage blockStorage) { switch (blockStorage) @@ -559,16 +547,12 @@ void CollectVariables::visitVariable(const TIntermSymbol *variable, interfaceBlock.layout = GetBlockLayoutType(blockType->blockStorage()); // Gather field information - const TFieldList &fieldList = blockType->fields(); - - for (size_t fieldIndex = 0; fieldIndex < fieldList.size(); ++fieldIndex) + for (const TField *field : blockType->fields()) { - const TField &field = *fieldList[fieldIndex]; - const TString &fullFieldName = InterfaceBlockFieldName(*blockType, field); - const TType &fieldType = *field.type(); + const TType &fieldType = *field->type(); NameHashingTraverser traverser(mHashFunction, mSymbolTable); - traverser.traverse(fieldType, fullFieldName, &interfaceBlock.fields); + traverser.traverse(fieldType, field->name(), &interfaceBlock.fields); interfaceBlock.fields.back().isRowMajorLayout = (fieldType.getLayoutQualifier().matrixPacking == EmpRowMajor); } diff --git a/src/libANGLE/Program.cpp b/src/libANGLE/Program.cpp index e21c8f401..748ceae03 100644 --- a/src/libANGLE/Program.cpp +++ b/src/libANGLE/Program.cpp @@ -2377,7 +2377,7 @@ void Program::defineUniformBlock(const sh::InterfaceBlock &interfaceBlock, GLenu // Track the first and last uniform index to determine the range of active uniforms in the // block. size_t firstBlockUniformIndex = mData.mUniforms.size(); - defineUniformBlockMembers(interfaceBlock.fields, "", blockIndex); + defineUniformBlockMembers(interfaceBlock.fields, interfaceBlock.fieldPrefix(), blockIndex); size_t lastBlockUniformIndex = mData.mUniforms.size(); std::vector blockUniformIndexes; diff --git a/src/libANGLE/renderer/d3d/ProgramD3D.cpp b/src/libANGLE/renderer/d3d/ProgramD3D.cpp index 37a288ea1..72c6f1a1a 100644 --- a/src/libANGLE/renderer/d3d/ProgramD3D.cpp +++ b/src/libANGLE/renderer/d3d/ProgramD3D.cpp @@ -2063,8 +2063,8 @@ size_t ProgramD3D::getUniformBlockInfo(const sh::InterfaceBlock &interfaceBlock) encoder = &hlslEncoder; } - GetUniformBlockInfo(interfaceBlock.fields, "", encoder, interfaceBlock.isRowMajorLayout, - &mBlockInfo); + GetUniformBlockInfo(interfaceBlock.fields, interfaceBlock.fieldPrefix(), encoder, + interfaceBlock.isRowMajorLayout, &mBlockInfo); return encoder->getBlockSize(); } diff --git a/src/tests/compiler_tests/CollectVariables_test.cpp b/src/tests/compiler_tests/CollectVariables_test.cpp index 0005790a6..f29374f54 100644 --- a/src/tests/compiler_tests/CollectVariables_test.cpp +++ b/src/tests/compiler_tests/CollectVariables_test.cpp @@ -7,6 +7,8 @@ // Some tests for shader inspection // +#include + #include "angle_gl.h" #include "gtest/gtest.h" #include "GLSLANG/ShaderLang.h" @@ -18,14 +20,10 @@ class CollectVariablesTest : public testing::Test { public: - CollectVariablesTest(GLenum shaderType) - : mShaderType(shaderType), - mTranslator(nullptr) - { - } + CollectVariablesTest(GLenum shaderType) : mShaderType(shaderType) {} protected: - virtual void SetUp() + void SetUp() override { ShBuiltInResources resources; ShInitBuiltInResources(&resources); @@ -34,16 +32,10 @@ class CollectVariablesTest : public testing::Test initTranslator(resources); } - virtual void TearDown() - { - SafeDelete(mTranslator); - } - void initTranslator(const ShBuiltInResources &resources) { - SafeDelete(mTranslator); - mTranslator = new TranslatorGLSL( - mShaderType, SH_GLES3_SPEC, SH_GLSL_COMPATIBILITY_OUTPUT); + mTranslator.reset( + new TranslatorGLSL(mShaderType, SH_GLES3_SPEC, SH_GLSL_COMPATIBILITY_OUTPUT)); ASSERT_TRUE(mTranslator->Init(resources)); } @@ -113,8 +105,14 @@ class CollectVariablesTest : public testing::Test *outResult = &outputVariable; } + void compile(const std::string &shaderString) + { + const char *shaderStrings[] = {shaderString.c_str()}; + ASSERT_TRUE(mTranslator->compile(shaderStrings, 1, SH_VARIABLES)); + } + GLenum mShaderType; - TranslatorGLSL *mTranslator; + std::unique_ptr mTranslator; }; class CollectVertexVariablesTest : public CollectVariablesTest @@ -139,8 +137,7 @@ TEST_F(CollectFragmentVariablesTest, SimpleOutputVar) " out_fragColor = vec4(1.0);\n" "}\n"; - const char *shaderStrings[] = { shaderString.c_str() }; - ASSERT_TRUE(mTranslator->compile(shaderStrings, 1, SH_VARIABLES)); + compile(shaderString); const auto &outputVariables = mTranslator->getOutputVariables(); ASSERT_EQ(1u, outputVariables.size()); @@ -165,8 +162,7 @@ TEST_F(CollectFragmentVariablesTest, LocationOutputVar) " out_fragColor = vec4(1.0);\n" "}\n"; - const char *shaderStrings[] = { shaderString.c_str() }; - ASSERT_TRUE(mTranslator->compile(shaderStrings, 1, SH_VARIABLES)); + compile(shaderString); const auto &outputVariables = mTranslator->getOutputVariables(); ASSERT_EQ(1u, outputVariables.size()); @@ -190,8 +186,7 @@ TEST_F(CollectVertexVariablesTest, LocationAttribute) " gl_Position = in_Position;\n" "}\n"; - const char *shaderStrings[] = { shaderString.c_str() }; - ASSERT_TRUE(mTranslator->compile(shaderStrings, 1, SH_VARIABLES)); + compile(shaderString); const std::vector &attributes = mTranslator->getAttributes(); ASSERT_EQ(1u, attributes.size()); @@ -217,8 +212,7 @@ TEST_F(CollectVertexVariablesTest, SimpleInterfaceBlock) " gl_Position = vec4(f, 0.0, 0.0, 1.0);\n" "}\n"; - const char *shaderStrings[] = { shaderString.c_str() }; - ASSERT_TRUE(mTranslator->compile(shaderStrings, 1, SH_VARIABLES)); + compile(shaderString); const std::vector &interfaceBlocks = mTranslator->getInterfaceBlocks(); ASSERT_EQ(1u, interfaceBlocks.size()); @@ -254,8 +248,7 @@ TEST_F(CollectVertexVariablesTest, SimpleInstancedInterfaceBlock) " gl_Position = vec4(blockInstance.f, 0.0, 0.0, 1.0);\n" "}\n"; - const char *shaderStrings[] = { shaderString.c_str() }; - ASSERT_TRUE(mTranslator->compile(shaderStrings, 1, SH_VARIABLES)); + compile(shaderString); const std::vector &interfaceBlocks = mTranslator->getInterfaceBlocks(); ASSERT_EQ(1u, interfaceBlocks.size()); @@ -266,6 +259,7 @@ TEST_F(CollectVertexVariablesTest, SimpleInstancedInterfaceBlock) EXPECT_FALSE(interfaceBlock.isRowMajorLayout); EXPECT_EQ(sh::BLOCKLAYOUT_SHARED, interfaceBlock.layout); EXPECT_EQ("b", interfaceBlock.name); + EXPECT_EQ("blockInstance", interfaceBlock.instanceName); EXPECT_TRUE(interfaceBlock.staticUse); ASSERT_EQ(1u, interfaceBlock.fields.size()); @@ -275,7 +269,7 @@ TEST_F(CollectVertexVariablesTest, SimpleInstancedInterfaceBlock) EXPECT_GLENUM_EQ(GL_HIGH_FLOAT, field.precision); EXPECT_TRUE(field.staticUse); EXPECT_GLENUM_EQ(GL_FLOAT, field.type); - EXPECT_EQ("b.f", field.name); + EXPECT_EQ("f", field.name); EXPECT_FALSE(field.isRowMajorLayout); EXPECT_TRUE(field.fields.empty()); } @@ -292,8 +286,7 @@ TEST_F(CollectVertexVariablesTest, StructInterfaceBlock) " gl_Position = vec4(s.f, 0.0, 0.0, 1.0);\n" "}\n"; - const char *shaderStrings[] = { shaderString.c_str() }; - ASSERT_TRUE(mTranslator->compile(shaderStrings, 1, SH_VARIABLES)); + compile(shaderString); const std::vector &interfaceBlocks = mTranslator->getInterfaceBlocks(); ASSERT_EQ(1u, interfaceBlocks.size()); @@ -336,8 +329,7 @@ TEST_F(CollectVertexVariablesTest, StructInstancedInterfaceBlock) " gl_Position = vec4(instanceName.s.f, 0.0, 0.0, 1.0);\n" "}\n"; - const char *shaderStrings[] = { shaderString.c_str() }; - ASSERT_TRUE(mTranslator->compile(shaderStrings, 1, SH_VARIABLES)); + compile(shaderString); const std::vector &interfaceBlocks = mTranslator->getInterfaceBlocks(); ASSERT_EQ(1u, interfaceBlocks.size()); @@ -348,6 +340,7 @@ TEST_F(CollectVertexVariablesTest, StructInstancedInterfaceBlock) EXPECT_FALSE(interfaceBlock.isRowMajorLayout); EXPECT_EQ(sh::BLOCKLAYOUT_SHARED, interfaceBlock.layout); EXPECT_EQ("b", interfaceBlock.name); + EXPECT_EQ("instanceName", interfaceBlock.instanceName); EXPECT_TRUE(interfaceBlock.staticUse); ASSERT_EQ(1u, interfaceBlock.fields.size()); @@ -356,7 +349,7 @@ TEST_F(CollectVertexVariablesTest, StructInstancedInterfaceBlock) EXPECT_TRUE(field.isStruct()); EXPECT_TRUE(field.staticUse); - EXPECT_EQ("b.s", field.name); + EXPECT_EQ("s", field.name); EXPECT_FALSE(field.isRowMajorLayout); const sh::ShaderVariable &member = field.fields[0]; @@ -380,8 +373,7 @@ TEST_F(CollectVertexVariablesTest, NestedStructRowMajorInterfaceBlock) " gl_Position = vec4(s.m);\n" "}\n"; - const char *shaderStrings[] = { shaderString.c_str() }; - ASSERT_TRUE(mTranslator->compile(shaderStrings, 1, SH_VARIABLES)); + compile(shaderString); const std::vector &interfaceBlocks = mTranslator->getInterfaceBlocks(); ASSERT_EQ(1u, interfaceBlocks.size()); @@ -423,8 +415,7 @@ TEST_F(CollectVertexVariablesTest, VaryingInterpolation) " vary = 1.0;\n" "}\n"; - const char *shaderStrings[] = { shaderString.c_str() }; - ASSERT_TRUE(mTranslator->compile(shaderStrings, 1, SH_VARIABLES)); + compile(shaderString); const std::vector &varyings = mTranslator->getVaryings(); ASSERT_EQ(2u, varyings.size()); @@ -654,3 +645,96 @@ TEST_F(CollectFragmentVariablesTest, OutputVarESSL1EXTBlendFuncExtendedSecondary EXPECT_GLENUM_EQ(GL_FLOAT_VEC4, outputVariable->type); EXPECT_GLENUM_EQ(GL_MEDIUM_FLOAT, outputVariable->precision); } + +static khronos_uint64_t SimpleTestHash(const char *str, size_t len) +{ + return static_cast(len); +} + +class CollectHashedVertexVariablesTest : public CollectVertexVariablesTest +{ + protected: + void SetUp() override + { + // Initialize the translate with a hash function + ShBuiltInResources resources; + ShInitBuiltInResources(&resources); + resources.HashFunction = SimpleTestHash; + initTranslator(resources); + } +}; + +TEST_F(CollectHashedVertexVariablesTest, InstancedInterfaceBlock) +{ + const std::string &shaderString = + "#version 300 es\n" + "uniform blockName {\n" + " float field;\n" + "} blockInstance;" + "void main() {\n" + " gl_Position = vec4(blockInstance.field, 0.0, 0.0, 1.0);\n" + "}\n"; + + compile(shaderString); + + const std::vector &interfaceBlocks = mTranslator->getInterfaceBlocks(); + ASSERT_EQ(1u, interfaceBlocks.size()); + + const sh::InterfaceBlock &interfaceBlock = interfaceBlocks[0]; + + EXPECT_EQ(0u, interfaceBlock.arraySize); + EXPECT_FALSE(interfaceBlock.isRowMajorLayout); + EXPECT_EQ(sh::BLOCKLAYOUT_SHARED, interfaceBlock.layout); + EXPECT_EQ("blockName", interfaceBlock.name); + EXPECT_EQ("blockInstance", interfaceBlock.instanceName); + EXPECT_EQ("webgl_9", interfaceBlock.mappedName); + EXPECT_TRUE(interfaceBlock.staticUse); + + ASSERT_EQ(1u, interfaceBlock.fields.size()); + + const sh::InterfaceBlockField &field = interfaceBlock.fields[0]; + + EXPECT_GLENUM_EQ(GL_HIGH_FLOAT, field.precision); + EXPECT_TRUE(field.staticUse); + EXPECT_GLENUM_EQ(GL_FLOAT, field.type); + EXPECT_EQ("field", field.name); + EXPECT_EQ("webgl_5", field.mappedName); + EXPECT_FALSE(field.isRowMajorLayout); + EXPECT_TRUE(field.fields.empty()); +} + +TEST_F(CollectHashedVertexVariablesTest, StructUniform) +{ + const std::string &shaderString = + "#version 300 es\n" + "struct sType {\n" + " float field;\n" + "};" + "uniform sType u;" + "void main() {\n" + " gl_Position = vec4(u.field, 0.0, 0.0, 1.0);\n" + "}\n"; + + compile(shaderString); + + const auto &uniforms = mTranslator->getUniforms(); + ASSERT_EQ(1u, uniforms.size()); + + const sh::Uniform &uniform = uniforms[0]; + + EXPECT_EQ(0u, uniform.arraySize); + EXPECT_EQ("u", uniform.name); + EXPECT_EQ("webgl_1", uniform.mappedName); + EXPECT_TRUE(uniform.staticUse); + + ASSERT_EQ(1u, uniform.fields.size()); + + const sh::ShaderVariable &field = uniform.fields[0]; + + EXPECT_GLENUM_EQ(GL_HIGH_FLOAT, field.precision); + // EXPECT_TRUE(field.staticUse); // we don't yet support struct static use + EXPECT_GLENUM_EQ(GL_FLOAT, field.type); + EXPECT_EQ("field", field.name); + EXPECT_EQ("webgl_5", field.mappedName); + EXPECT_TRUE(field.fields.empty()); +}