Vulkan:Allow same-named var in nested scope

src/compiler.gni

@@ -198,6 +198,8 @@ angle_translator_sources = [
   "src/compiler/translator/tree_util/NodeSearch.h",
   "src/compiler/translator/tree_util/ReplaceVariable.cpp",
   "src/compiler/translator/tree_util/ReplaceVariable.h",
+  "src/compiler/translator/tree_util/ReplaceShadowingVariables.cpp",
+  "src/compiler/translator/tree_util/ReplaceShadowingVariables.h",
   "src/compiler/translator/tree_util/RunAtTheEndOfShader.cpp",
   "src/compiler/translator/tree_util/RunAtTheEndOfShader.h",
   "src/compiler/translator/tree_util/Visit.h",

src/compiler/translator/Compiler.cpp

@@ -49,6 +49,7 @@
 #include "compiler/translator/tree_ops/VectorizeVectorScalarArithmetic.h"
 #include "compiler/translator/tree_util/BuiltIn_autogen.h"
 #include "compiler/translator/tree_util/IntermNodePatternMatcher.h"
+#include "compiler/translator/tree_util/ReplaceShadowingVariables.h"
 #include "compiler/translator/util.h"
 #include "third_party/compiler/ArrayBoundsClamper.h"
 
@@ -537,6 +538,10 @@ bool TCompiler::checkAndSimplifyAST(TIntermBlock *root,
             return false;
         }
     }
+    if (IsSpecWithFunctionBodyNewScope(mShaderSpec, mShaderVersion))
+    {
+        ReplaceShadowingVariables(root, &mSymbolTable);
+    }
 
     if (mShaderVersion >= 310 && !ValidateVaryingLocations(root, &mDiagnostics, mShaderType))
     {

src/compiler/translator/ParseContext.cpp

@@ -214,7 +214,8 @@ TParseContext::TParseContext(TSymbolTable &symt,
       mGeometryShaderInvocations(0),
       mGeometryShaderMaxVertices(-1),
       mMaxGeometryShaderInvocations(resources.MaxGeometryShaderInvocations),
-      mMaxGeometryShaderMaxVertices(resources.MaxGeometryOutputVertices)
+      mMaxGeometryShaderMaxVertices(resources.MaxGeometryOutputVertices),
+      mFunctionBodyNewScope(false)
 {}
 
 TParseContext::~TParseContext() {}
@@ -3262,6 +3263,13 @@ TIntermFunctionDefinition *TParseContext::addFunctionDefinition(
     TIntermBlock *functionBody,
     const TSourceLoc &location)
 {
+    // Undo push at end of parseFunctionDefinitionHeader() below for ESSL1.00 case
+    if (mFunctionBodyNewScope)
+    {
+        mFunctionBodyNewScope = false;
+        symbolTable.pop();
+    }
+
     // Check that non-void functions have at least one return statement.
     if (mCurrentFunctionType->getBasicType() != EbtVoid && !mFunctionReturnsValue)
     {
@@ -3301,6 +3309,13 @@ void TParseContext::parseFunctionDefinitionHeader(const TSourceLoc &location,
 
     *prototypeOut = createPrototypeNodeFromFunction(*function, location, true);
     setLoopNestingLevel(0);
+
+    // ESSL 1.00 spec allows for variable in function body to redefine parameter
+    if (IsSpecWithFunctionBodyNewScope(mShaderSpec, mShaderVersion))
+    {
+        mFunctionBodyNewScope = true;
+        symbolTable.push();
+    }
 }
 
 TFunction *TParseContext::parseFunctionDeclarator(const TSourceLoc &location, TFunction *function)

src/compiler/translator/ParseContext.h

@@ -650,6 +650,9 @@ class TParseContext : angle::NonCopyable
     int mGeometryShaderMaxVertices;
     int mMaxGeometryShaderInvocations;
     int mMaxGeometryShaderMaxVertices;
+
+    // Track when we add new scope for func body in ESSL 1.00 spec
+    bool mFunctionBodyNewScope;
 };
 
 int PaParseStrings(size_t count,

src/compiler/translator/tree_util/ReplaceShadowingVariables.cpp

+//
+// Copyright 2019 The ANGLE Project Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// ReplaceShadowingVariables.cpp: Replace all references to any variable in the AST that is
+// a redefinition of a variable in a nested scope. This is a useful for ESSL 1.00 shaders
+// where the spec section "4.2.3. Redeclaring Variables" states "However, a nested scope can
+// override an outer scope's declaration of a particular variable name." This is changed in
+// later spec versions, such as ESSL 3.20 spec which states "If [a variable] is declared as
+// a parameter in a function definition, it is scoped until the end of that function
+// definition. A function's parameter declarations and body together form a single scope."
+//
+// So this class is useful when translating from ESSL 1.00 shaders, where function body var
+// redefinition is allowed, to later shader versions where it's not allowed.
+//
+
+#include "compiler/translator/tree_util/ReplaceShadowingVariables.h"
+#include "compiler/translator/tree_util/ReplaceVariable.h"
+
+#include "compiler/translator/IntermNode.h"
+#include "compiler/translator/Symbol.h"
+#include "compiler/translator/SymbolTable.h"
+#include "compiler/translator/tree_util/IntermNode_util.h"
+#include "compiler/translator/tree_util/IntermTraverse.h"
+
+#include <unordered_set>
+
+namespace sh
+{
+
+namespace
+{
+
+// Custom struct to queue up any replacements until after AST traversal
+struct DeferredReplacementBlock
+{
+    const TVariable *originalVariable;  // variable to be replaced
+    TVariable *replacementVariable;     // variable to replace originalVar with
+    TIntermBlock *functionBody;         // function body where replacement occurs
+};
+
+class ReplaceShadowingVariablesTraverser : public TIntermTraverser
+{
+  public:
+    ReplaceShadowingVariablesTraverser(TSymbolTable *symbolTable)
+        : TIntermTraverser(true, true, true),
+          mSymbolTable(symbolTable),
+          mParameterNames{},
+          mFunctionBody(nullptr)
+    {}
+
+    bool visitFunctionDefinition(Visit visit, TIntermFunctionDefinition *node) override
+    {
+        // In pre-visit of function, record params
+        if (visit == PreVisit)
+        {
+            ASSERT(mParameterNames.size() == 0);
+            const TFunction *func = node->getFunctionPrototype()->getFunction();
+            // Grab all of the parameter names from the function prototype
+            uint32_t paramCount = func->getParamCount();
+            for (uint32_t i = 0; i < paramCount; ++i)
+            {
+                mParameterNames.emplace(std::string(func->getParam(i)->name().data()));
+            }
+            if (mParameterNames.size() > 0)
+                mFunctionBody = node->getBody();
+        }
+        else if (visit == PostVisit)
+        {
+            // Clear data saved from function definition
+            mParameterNames.clear();
+            mFunctionBody = nullptr;
+        }
+        return true;
+    }
+    bool visitDeclaration(Visit visit, TIntermDeclaration *node) override
+    {
+        if (visit == PreVisit && mParameterNames.size() != 0)
+        {
+            TIntermSequence *decls = node->getSequence();
+            for (auto &declVector : *decls)
+            {
+                // no init case
+                TIntermSymbol *symNode = declVector->getAsSymbolNode();
+                if (symNode == nullptr)
+                {
+                    // init case
+                    TIntermBinary *binaryNode = declVector->getAsBinaryNode();
+                    ASSERT(binaryNode->getOp() == EOpInitialize);
+                    symNode = binaryNode->getLeft()->getAsSymbolNode();
+                }
+                ASSERT(symNode != nullptr);
+                std::string varName = std::string(symNode->variable().name().data());
+                if (mParameterNames.count(varName) > 0)
+                {
+                    // We found a redefined var so queue replacement
+                    mReplacements.emplace_back(DeferredReplacementBlock{
+                        &symNode->variable(),
+                        CreateTempVariable(mSymbolTable, &symNode->variable().getType()),
+                        mFunctionBody});
+                }
+            }
+        }
+        return true;
+    }
+    // Perform replacement of vars for any deferred replacements that were identified
+    void executeReplacements()
+    {
+        for (DeferredReplacementBlock &replace : mReplacements)
+        {
+            ReplaceVariable(replace.functionBody, replace.originalVariable,
+                            replace.replacementVariable);
+        }
+        mReplacements.clear();
+    }
+
+  private:
+    TSymbolTable *mSymbolTable;
+    std::unordered_set<std::string> mParameterNames;
+    TIntermBlock *mFunctionBody;
+    std::vector<DeferredReplacementBlock> mReplacements;
+};
+
+}  // anonymous namespace
+
+// Replaces every occurrence of a variable with another variable.
+void ReplaceShadowingVariables(TIntermBlock *root, TSymbolTable *symbolTable)
+{
+    ReplaceShadowingVariablesTraverser traverser(symbolTable);
+    root->traverse(&traverser);
+    traverser.executeReplacements();
+    traverser.updateTree();
+}
+
+}  // namespace sh

src/compiler/translator/tree_util/ReplaceShadowingVariables.h

+//
+// Copyright 2019 The ANGLE Project Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// ReplaceShadowingVariables.h: Find any variables that are redefined within a nested
+// scope and replace them with a newly named variable.
+
+#ifndef COMPILER_TRANSLATOR_TREEUTIL_REPLACESHADOWINGVARIABLES_H_
+#define COMPILER_TRANSLATOR_TREEUTIL_REPLACESHADOWINGVARIABLES_H_
+
+namespace sh
+{
+
+class TIntermBlock;
+class TSymbolTable;
+
+void ReplaceShadowingVariables(TIntermBlock *root, TSymbolTable *symbolTable);
+}  // namespace sh
+
+#endif  // COMPILER_TRANSLATOR_TREEUTIL_REPLACESHADOWINGVARIABLES_H_

src/compiler/translator/util.cpp

@@ -818,4 +818,9 @@ GLenum GetImageInternalFormatType(TLayoutImageInternalFormat iifq)
     }
 }
 
+bool IsSpecWithFunctionBodyNewScope(ShShaderSpec shaderSpec, int shaderVersion)
+{
+    return (shaderVersion == 100 && !sh::IsWebGLBasedSpec(shaderSpec));
+}
+
 }  // namespace sh

src/compiler/translator/util.h

@@ -68,6 +68,8 @@ bool IsOutputVulkan(ShShaderOutput output);
 bool IsInShaderStorageBlock(TIntermTyped *node);
 
 GLenum GetImageInternalFormatType(TLayoutImageInternalFormat iifq);
+// ESSL 1.00 shaders nest function body scope within function parameter scope
+bool IsSpecWithFunctionBodyNewScope(ShShaderSpec shaderSpec, int shaderVersion);
 }  // namespace sh
 
 #endif  // COMPILER_TRANSLATOR_UTIL_H_

src/tests/deqp_support/deqp_gles2_test_expectations.txt

@@ -83,8 +83,6 @@
 
 // Shader failures.
 3434 NEXUS5X GLES : dEQP-GLES2.functional.shaders.preprocessor.pragmas.pragma_* = FAIL
-3287 : dEQP-GLES2.functional.shaders.scoping.valid.local_variable_hides_function_parameter_vertex = FAIL
-3287 : dEQP-GLES2.functional.shaders.scoping.valid.local_variable_hides_function_parameter_fragment = FAIL
 
 // The fragment_ops.depth_stencil.random tests all seem to fail on D3D11.
 3282 D3D11 : dEQP-GLES2.functional.fragment_ops.depth_stencil.random.* = FAIL