Skip to content

A
angle
Commit 2e2f3b79 by Jamie Madill
Options

Revert "Remove dynamic indexing of matrices and vectors in HLSL"

This reverts commit 3766a40d. This CL was causing crashes in UniformHLSL.cpp, where an internal uniform "base" was attempted to be declared in HLSL. Was crashing on an external WebGL 3D canvas page (http://www.taccgl.org/?dbg=t). BUG=546686 Original commit message: Re-landing after fixing D3D9 specific issues. HLSL doesn't support dynamic indexing of matrices and vectors, so replace that with helper functions that unroll dynamic indexing into switch/case and static indexing. Both the indexed vector/matrix expression and the index may have side effects, and these will be evaluated correctly. If necessary, index expressions that have side effects will be written to a temporary variable that will replace the index. Besides dEQP tests, this change is tested by a WebGL 2 conformance test. In the case that a dynamic index is out-of-range, the base ESSL 3.00 spec allows undefined behavior. KHR_robust_buffer_access_behavior adds the requirement that program termination should not occur and that out-of-range reads must return either a value from the active program's memory or zero, and out-of-range writes should only affect the active program's memory or do nothing. This patch clamps out-of-range indices so that either the first or last item of the matrix/vector is accessed. The code is not transformed in case the it fits within the limited subset of ESSL 1.00 given in Appendix A of the spec. If the code isn't within the restricted subset, even ESSL 1.00 shaders may require this workaround. BUG=angleproject:1116 TEST=dEQP-GLES3.functional.shaders.indexing.* (all pass after change) WebGL 2 conformance tests (glsl3/vector-dynamic-indexing.html) Change-Id: I1d4b2e3888e91af7d5eebf743d12778698b6b903 Reviewed-on: https://chromium-review.googlesource.com/310270Reviewed-by: 's avatarJamie Madill <jmadill@chromium.org> Tested-by: 's avatarJamie Madill <jmadill@chromium.org>
parent e623bd46
Showing with 36 additions and 84 deletions
src/compiler.gypi
......@@ -159,8 +159,6 @@
'compiler/translator/BuiltInFunctionEmulatorHLSL.h',
'compiler/translator/OutputHLSL.cpp',
'compiler/translator/OutputHLSL.h',
'compiler/translator/RemoveDynamicIndexing.cpp',
'compiler/translator/RemoveDynamicIndexing.h',
'compiler/translator/RemoveSwitchFallThrough.cpp',
'compiler/translator/RemoveSwitchFallThrough.h',
'compiler/translator/RewriteElseBlocks.cpp',
......
src/compiler/translator/Compiler.cpp
......@@ -153,15 +153,6 @@ TCompiler::~TCompiler()
{
}
bool TCompiler::shouldRunLoopAndIndexingValidation(int compileOptions) const
{
// If compiling an ESSL 1.00 shader for WebGL, or if its been requested through the API,
// validate loop and indexing as well (to verify that the shader only uses minimal functionality
// of ESSL 1.00 as in Appendix A of the spec).
return (IsWebGLBasedSpec(shaderSpec) && shaderVersion == 100) ||
(compileOptions & SH_VALIDATE_LOOP_INDEXING);
}
bool TCompiler::Init(const ShBuiltInResources& resources)
{
shaderVersion = 100;
......@@ -239,6 +230,12 @@ TIntermNode *TCompiler::compileTreeImpl(const char *const shaderStrings[],
success = false;
}
// If compiling an ESSL 1.00 shader for WebGL, or if its been requested through the API,
// validate loop and indexing as well (to verify that the shader only uses minimal functionality
// of ESSL 1.00 as in Appendix A of the spec).
bool validateLoopAndIndexing = (IsWebGLBasedSpec(shaderSpec) && shaderVersion == 100) ||
(compileOptions & SH_VALIDATE_LOOP_INDEXING);
TIntermNode *root = nullptr;
if (success)
......@@ -281,7 +278,7 @@ TIntermNode *TCompiler::compileTreeImpl(const char *const shaderStrings[],
if (success && shaderVersion == 300 && shaderType == GL_FRAGMENT_SHADER)
success = validateOutputs(root);
if (success && shouldRunLoopAndIndexingValidation(compileOptions))
if (success && validateLoopAndIndexing)
success = validateLimitations(root);
if (success && (compileOptions & SH_TIMING_RESTRICTIONS))
......
src/compiler/translator/Compiler.h
......@@ -101,8 +101,6 @@ class TCompiler : public TShHandleBase
ShShaderOutput getOutputType() const { return outputType; }
const std::string &getBuiltInResourcesString() const { return builtInResourcesString; }
bool shouldRunLoopAndIndexingValidation(int compileOptions) const;
// Get the resources set by InitBuiltInSymbolTable
const ShBuiltInResources& getResources() const;
......
src/compiler/translator/IntermNode.cpp
......@@ -311,13 +311,17 @@ bool TIntermAggregate::replaceChildNodeWithMultiple(TIntermNode *original, TInte
bool TIntermAggregate::insertChildNodes(TIntermSequence::size_type position, TIntermSequence insertions)
{
if (position > mSequence.size())
TIntermSequence::size_type itPosition = 0;
for (auto it = mSequence.begin(); it < mSequence.end(); ++it)
{
return false;
if (itPosition == position)
{
mSequence.insert(it, insertions.begin(), insertions.end());
return true;
}
++itPosition;
}
auto it = mSequence.begin() + position;
mSequence.insert(it, insertions.begin(), insertions.end());
return true;
return false;
}
void TIntermAggregate::setPrecisionFromChildren()
......@@ -2544,20 +2548,9 @@ void TIntermTraverser::updateTree()
{
const NodeInsertMultipleEntry &insertion = mInsertions[ii];
ASSERT(insertion.parent);
if (!insertion.insertionsAfter.empty())
{
bool inserted = insertion.parent->insertChildNodes(insertion.position + 1,
insertion.insertionsAfter);
ASSERT(inserted);
UNUSED_ASSERTION_VARIABLE(inserted);
}
if (!insertion.insertionsBefore.empty())
{
bool inserted =
insertion.parent->insertChildNodes(insertion.position, insertion.insertionsBefore);
ASSERT(inserted);
UNUSED_ASSERTION_VARIABLE(inserted);
}
bool inserted = insertion.parent->insertChildNodes(insertion.position, insertion.insertions);
ASSERT(inserted);
UNUSED_ASSERTION_VARIABLE(inserted);
}
for (size_t ii = 0; ii < mReplacements.size(); ++ii)
{
......
src/compiler/translator/IntermNode.h
......@@ -781,21 +781,16 @@ class TIntermTraverser : angle::NonCopyable
// To insert multiple nodes on the parent aggregate node
struct NodeInsertMultipleEntry
{
NodeInsertMultipleEntry(TIntermAggregate *_parent,
TIntermSequence::size_type _position,
TIntermSequence _insertionsBefore,
TIntermSequence _insertionsAfter)
NodeInsertMultipleEntry(TIntermAggregate *_parent, TIntermSequence::size_type _position, TIntermSequence _insertions)
: parent(_parent),
position(_position),
insertionsBefore(_insertionsBefore),
insertionsAfter(_insertionsAfter)
position(_position),
insertions(_insertions)
{
}
TIntermAggregate *parent;
TIntermSequence::size_type position;
TIntermSequence insertionsBefore;
TIntermSequence insertionsAfter;
TIntermSequence insertions;
};
// During traversing, save all the changes that need to happen into
......@@ -812,11 +807,6 @@ class TIntermTraverser : angle::NonCopyable
// supported.
void insertStatementsInParentBlock(const TIntermSequence &insertions);
// Same as above, but supports simultaneous insertion of statements before and after the node
// currently being traversed.
void insertStatementsInParentBlock(const TIntermSequence &insertionsBefore,
const TIntermSequence &insertionsAfter);
// Helper to create a temporary symbol node with the given qualifier.
TIntermSymbol *createTempSymbol(const TType &type, TQualifier qualifier);
// Helper to create a temporary symbol node.
......
src/compiler/translator/IntermTraverse.cpp
......@@ -81,16 +81,8 @@ void TIntermTraverser::popParentBlock()
void TIntermTraverser::insertStatementsInParentBlock(const TIntermSequence &insertions)
{
TIntermSequence emptyInsertionsAfter;
insertStatementsInParentBlock(insertions, emptyInsertionsAfter);
}
void TIntermTraverser::insertStatementsInParentBlock(const TIntermSequence &insertionsBefore,
const TIntermSequence &insertionsAfter)
{
ASSERT(!mParentBlockStack.empty());
NodeInsertMultipleEntry insert(mParentBlockStack.back().node, mParentBlockStack.back().pos,
insertionsBefore, insertionsAfter);
NodeInsertMultipleEntry insert(mParentBlockStack.back().node, mParentBlockStack.back().pos, insertions);
mInsertions.push_back(insert);
}
......
src/compiler/translator/RemoveDynamicIndexing.h deleted 100644 → 0
//
// Copyright (c) 2002-2015 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.
//
// RemoveDynamicIndexing is an AST traverser to remove dynamic indexing of vectors and matrices,
// replacing them with calls to functions that choose which component to return or write.
//
#ifndef COMPILER_TRANSLATOR_REMOVEDYNAMICINDEXING_H_
#define COMPILER_TRANSLATOR_REMOVEDYNAMICINDEXING_H_
class TIntermNode;
class TSymbolTable;
void RemoveDynamicIndexing(TIntermNode *root,
unsigned int *temporaryIndex,
const TSymbolTable &symbolTable,
int shaderVersion);
#endif // COMPILER_TRANSLATOR_REMOVEDYNAMICINDEXING_H_
src/compiler/translator/TranslatorHLSL.cpp
......@@ -8,7 +8,6 @@
#include "compiler/translator/ArrayReturnValueToOutParameter.h"
#include "compiler/translator/OutputHLSL.h"
#include "compiler/translator/RemoveDynamicIndexing.h"
#include "compiler/translator/RewriteElseBlocks.h"
#include "compiler/translator/SeparateArrayInitialization.h"
#include "compiler/translator/SeparateDeclarations.h"
......@@ -39,12 +38,6 @@ void TranslatorHLSL::translate(TIntermNode *root, int compileOptions)
// as a return value to use an out parameter to transfer the array data instead.
ArrayReturnValueToOutParameter(root, getTemporaryIndex());
if (!shouldRunLoopAndIndexingValidation(compileOptions))
{
// HLSL doesn't support dynamic indexing of vectors and matrices.
RemoveDynamicIndexing(root, getTemporaryIndex(), getSymbolTable(), getShaderVersion());
}
// Work around D3D9 bug that would manifest in vertex shaders with selection blocks which
// use a vertex attribute as a condition, and some related computation in the else block.
if (getOutputType() == SH_HLSL9_OUTPUT && getShaderType() == GL_VERTEX_SHADER)
......
src/tests/deqp_support/deqp_gles3_test_expectations.txt
......@@ -54,6 +54,18 @@
1089 WIN : dEQP-GLES3.functional.shaders.functions.invalid.local_function_prototype_fragment = FAIL
1089 WIN : dEQP-GLES3.functional.shaders.functions.invalid.overload_builtin_function_vertex = FAIL
1089 WIN : dEQP-GLES3.functional.shaders.functions.invalid.overload_builtin_function_fragment = FAIL
1090 WIN : dEQP-GLES3.functional.shaders.indexing.vector_subscript.vec2_dynamic_subscript_write_* = FAIL
1090 WIN : dEQP-GLES3.functional.shaders.indexing.vector_subscript.vec3_dynamic_subscript_write_* = FAIL
1090 WIN : dEQP-GLES3.functional.shaders.indexing.vector_subscript.vec4_dynamic_subscript_write_* = FAIL
1090 WIN : dEQP-GLES3.functional.shaders.indexing.matrix_subscript.mat2_dynamic_write_* = FAIL
1090 WIN : dEQP-GLES3.functional.shaders.indexing.matrix_subscript.mat2x3_dynamic_write_* = FAIL
1090 WIN : dEQP-GLES3.functional.shaders.indexing.matrix_subscript.mat2x4_dynamic_write_* = FAIL
1090 WIN : dEQP-GLES3.functional.shaders.indexing.matrix_subscript.mat3x2_dynamic_write_* = FAIL
1090 WIN : dEQP-GLES3.functional.shaders.indexing.matrix_subscript.mat3_dynamic_write_* = FAIL
1090 WIN : dEQP-GLES3.functional.shaders.indexing.matrix_subscript.mat3x4_dynamic_write_* = FAIL
1090 WIN : dEQP-GLES3.functional.shaders.indexing.matrix_subscript.mat4x2_dynamic_write_* = FAIL
1090 WIN : dEQP-GLES3.functional.shaders.indexing.matrix_subscript.mat4x3_dynamic_write_* = FAIL
1090 WIN : dEQP-GLES3.functional.shaders.indexing.matrix_subscript.mat4_dynamic_write_* = FAIL
1091 WIN : dEQP-GLES3.functional.shaders.loops.for_constant_iterations.nested_sequence_vertex = FAIL
1091 WIN : dEQP-GLES3.functional.shaders.loops.for_constant_iterations.nested_sequence_fragment = FAIL
1091 WIN : dEQP-GLES3.functional.shaders.loops.for_constant_iterations.nested_tricky_dataflow_1_vertex = FAIL
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment