Workaround the unary minus operator issue on Intel
On some Intel D3D drivers, evaluating unary minor operator on an
integer variable may get wrong answer in vertex shader.
This patch works around this bug by replacing -(int) with ~(int)+1
on Windows Intel.
BUG=chromium:644033
Change-Id: I0af719e84d618a33f25bcb33bde0c381fb462a31
Reviewed-on: https://chromium-review.googlesource.com/381675
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Commit-Queue: Jamie Madill <jmadill@chromium.org>
diff --git a/src/compiler.gypi b/src/compiler.gypi
index 8d754ee..c37ac6d 100644
--- a/src/compiler.gypi
+++ b/src/compiler.gypi
@@ -98,6 +98,8 @@
'compiler/translator/RewriteDoWhile.h',
'compiler/translator/RewriteTexelFetchOffset.cpp',
'compiler/translator/RewriteTexelFetchOffset.h',
+ 'compiler/translator/RewriteUnaryMinusOperatorInt.cpp',
+ 'compiler/translator/RewriteUnaryMinusOperatorInt.h',
'compiler/translator/ScalarizeVecAndMatConstructorArgs.cpp',
'compiler/translator/ScalarizeVecAndMatConstructorArgs.h',
'compiler/translator/SearchSymbol.cpp',
diff --git a/src/compiler/translator/RewriteUnaryMinusOperatorInt.cpp b/src/compiler/translator/RewriteUnaryMinusOperatorInt.cpp
new file mode 100644
index 0000000..ef708cb
--- /dev/null
+++ b/src/compiler/translator/RewriteUnaryMinusOperatorInt.cpp
@@ -0,0 +1,112 @@
+//
+// Copyright (c) 2016 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.
+//
+// Implementation of evaluating unary integer variable bug workaround.
+// See header for more info.
+
+#include "compiler/translator/RewriteUnaryMinusOperatorInt.h"
+
+#include "compiler/translator/IntermNode.h"
+
+namespace sh
+{
+
+namespace
+{
+
+class Traverser : public TIntermTraverser
+{
+ public:
+ static void Apply(TIntermNode *root);
+
+ private:
+ Traverser();
+ bool visitUnary(Visit visit, TIntermUnary *node) override;
+ void nextIteration();
+
+ bool mFound = false;
+};
+
+// static
+void Traverser::Apply(TIntermNode *root)
+{
+ Traverser traverser;
+ do
+ {
+ traverser.nextIteration();
+ root->traverse(&traverser);
+ if (traverser.mFound)
+ {
+ traverser.updateTree();
+ }
+ } while (traverser.mFound);
+}
+
+Traverser::Traverser() : TIntermTraverser(true, false, false)
+{
+}
+
+void Traverser::nextIteration()
+{
+ mFound = false;
+}
+
+bool Traverser::visitUnary(Visit visit, TIntermUnary *node)
+{
+ if (mFound)
+ {
+ return false;
+ }
+
+ // Decide if the current unary operator is unary minus.
+ if (node->getOp() != EOpNegative)
+ {
+ return true;
+ }
+
+ // Decide if the current operand is an integer variable.
+ TIntermTyped *opr = node->getOperand();
+ if (!opr->getType().isScalarInt())
+ {
+ return true;
+ }
+
+ // Potential problem case detected, apply workaround: -(int) -> ~(int) + 1.
+ // ~(int)
+ TIntermUnary *bitwiseNot = new TIntermUnary(EOpBitwiseNot, opr);
+ bitwiseNot->setLine(opr->getLine());
+
+ // Constant 1 (or 1u)
+ TConstantUnion *one = new TConstantUnion();
+ if (opr->getType().getBasicType() == EbtInt)
+ {
+ one->setIConst(1);
+ }
+ else
+ {
+ one->setUConst(1u);
+ }
+ TIntermConstantUnion *oneNode = new TIntermConstantUnion(one, opr->getType());
+ oneNode->getTypePointer()->setQualifier(EvqConst);
+ oneNode->setLine(opr->getLine());
+
+ // ~(int) + 1
+ TIntermBinary *add = new TIntermBinary(EOpAdd, bitwiseNot, oneNode);
+ add->setLine(opr->getLine());
+
+ queueReplacement(node, add, OriginalNode::IS_DROPPED);
+
+ mFound = true;
+ return false;
+}
+
+} // anonymous namespace
+
+void RewriteUnaryMinusOperatorInt(TIntermNode *root)
+{
+ Traverser::Apply(root);
+}
+
+} // namespace sh
\ No newline at end of file
diff --git a/src/compiler/translator/RewriteUnaryMinusOperatorInt.h b/src/compiler/translator/RewriteUnaryMinusOperatorInt.h
new file mode 100644
index 0000000..50f0c44
--- /dev/null
+++ b/src/compiler/translator/RewriteUnaryMinusOperatorInt.h
@@ -0,0 +1,20 @@
+// Copyright (c) 2016 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.
+//
+// This mutating tree traversal works around a bug on evaluating unary
+// integer variable on Intel D3D driver. It works by rewriting -(int) to
+// ~(int) + 1 when evaluating unary integer variables.
+
+#ifndef COMPILER_TRANSLATOR_REWRITEUNARYMINUSOPERATORINT_H_
+#define COMPILER_TRANSLATOR_REWRITEUNARYMINUSOPERATORINT_H_
+
+class TIntermNode;
+namespace sh
+{
+
+void RewriteUnaryMinusOperatorInt(TIntermNode *root);
+
+} // namespace sh
+
+#endif // COMPILER_TRANSLATOR_REWRITEUNARYMINUSOPERATORINT_H_
\ No newline at end of file
diff --git a/src/compiler/translator/TranslatorHLSL.cpp b/src/compiler/translator/TranslatorHLSL.cpp
index ab77049..0785c9e 100644
--- a/src/compiler/translator/TranslatorHLSL.cpp
+++ b/src/compiler/translator/TranslatorHLSL.cpp
@@ -16,6 +16,7 @@
#include "compiler/translator/RemoveDynamicIndexing.h"
#include "compiler/translator/RewriteElseBlocks.h"
#include "compiler/translator/RewriteTexelFetchOffset.h"
+#include "compiler/translator/RewriteUnaryMinusOperatorInt.h"
#include "compiler/translator/SeparateArrayInitialization.h"
#include "compiler/translator/SeparateDeclarations.h"
#include "compiler/translator/SeparateExpressionsReturningArrays.h"
@@ -105,6 +106,12 @@
getShaderVersion());
}
+ if (((compileOptions & SH_REWRITE_INTEGER_UNARY_MINUS_OPERATOR) != 0) &&
+ getShaderType() == GL_VERTEX_SHADER)
+ {
+ sh::RewriteUnaryMinusOperatorInt(root);
+ }
+
sh::OutputHLSL outputHLSL(getShaderType(), getShaderVersion(), getExtensionBehavior(),
getSourcePath(), getOutputType(), numRenderTargets, getUniforms(), compileOptions);
diff --git a/src/libANGLE/renderer/d3d/ShaderD3D.cpp b/src/libANGLE/renderer/d3d/ShaderD3D.cpp
index b4e89b1..9930791 100644
--- a/src/libANGLE/renderer/d3d/ShaderD3D.cpp
+++ b/src/libANGLE/renderer/d3d/ShaderD3D.cpp
@@ -59,6 +59,10 @@
{
mAdditionalOptions |= SH_REWRITE_TEXELFETCHOFFSET_TO_TEXELFETCH;
}
+ if (workarounds.rewriteUnaryMinusOperator)
+ {
+ mAdditionalOptions |= SH_REWRITE_INTEGER_UNARY_MINUS_OPERATOR;
+ }
}
ShaderD3D::~ShaderD3D()
diff --git a/src/libANGLE/renderer/d3d/WorkaroundsD3D.h b/src/libANGLE/renderer/d3d/WorkaroundsD3D.h
index 06d081e..a3f7346 100644
--- a/src/libANGLE/renderer/d3d/WorkaroundsD3D.h
+++ b/src/libANGLE/renderer/d3d/WorkaroundsD3D.h
@@ -72,7 +72,7 @@
// is negative, even if the sum of Offset and Location is in range. This may cause errors when
// translating GLSL's function texelFetchOffset into texture.Load, as it is valid for
// texelFetchOffset to use negative texture coordinates as its parameter P when the sum of P
- // and Offset is in range. To work around this, we translatie texelFetchOffset into texelFetch
+ // and Offset is in range. To work around this, we translate texelFetchOffset into texelFetch
// by adding Offset directly to Location before reading the texture.
bool preAddTexelFetchOffsets = false;
@@ -85,6 +85,10 @@
// This workaroud will disable B5G6R5 support when it's Intel driver. By default, it will use
// R8G8B8A8 format.
bool disableB5G6R5Support = false;
+
+ // On some Intel drivers, evaluating unary minus operator on integer may get wrong answer in
+ // vertex shaders. To work around this bug, we translate -(int) into ~(int)+1.
+ bool rewriteUnaryMinusOperator = false;
};
} // namespace rx
diff --git a/src/libANGLE/renderer/d3d/d3d11/renderer11_utils.cpp b/src/libANGLE/renderer/d3d/d3d11/renderer11_utils.cpp
index 82223be..8112a7d 100644
--- a/src/libANGLE/renderer/d3d/d3d11/renderer11_utils.cpp
+++ b/src/libANGLE/renderer/d3d/d3d11/renderer11_utils.cpp
@@ -1542,6 +1542,7 @@
workarounds.preAddTexelFetchOffsets = (adapterDesc.VendorId == VENDOR_ID_INTEL);
workarounds.disableB5G6R5Support = (adapterDesc.VendorId == VENDOR_ID_INTEL);
+ workarounds.rewriteUnaryMinusOperator = (adapterDesc.VendorId == VENDOR_ID_INTEL);
// TODO(jmadill): Disable when we have a fixed driver version.
workarounds.emulateTinyStencilTextures = (adapterDesc.VendorId == VENDOR_ID_AMD);
diff --git a/src/tests/gl_tests/GLSLTest.cpp b/src/tests/gl_tests/GLSLTest.cpp
index 49b6c74..2a5ac12 100644
--- a/src/tests/gl_tests/GLSLTest.cpp
+++ b/src/tests/gl_tests/GLSLTest.cpp
@@ -2241,6 +2241,104 @@
EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
}
+// Convers a bug with the unary minus operator on signed integer workaround.
+TEST_P(GLSLTest_ES3, UnaryMinusOperatorSignedInt)
+{
+ const std::string &vert =
+ "#version 300 es\n"
+ "in highp vec4 position;\n"
+ "out mediump vec4 v_color;\n"
+ "uniform int ui_one;\n"
+ "uniform int ui_two;\n"
+ "uniform int ui_three;\n"
+ "void main() {\n"
+ " int s[3];\n"
+ " s[0] = ui_one;\n"
+ " s[1] = -(-(-ui_two + 1) + 1);\n" // s[1] = -ui_two
+ " s[2] = ui_three;\n"
+ " int result = 0;\n"
+ " for (int i = 0; i < ui_three; i++) {\n"
+ " result += s[i];\n"
+ " }\n"
+ " v_color = (result == 2) ? vec4(0, 1, 0, 1) : vec4(1, 0, 0, 1);\n"
+ " gl_Position = position;\n"
+ "}\n";
+ const std::string &frag =
+ "#version 300 es\n"
+ "in mediump vec4 v_color;\n"
+ "layout(location=0) out mediump vec4 o_color;\n"
+ "void main() {\n"
+ " o_color = v_color;\n"
+ "}\n";
+
+ ANGLE_GL_PROGRAM(prog, vert, frag);
+
+ gl::Context *context = reinterpret_cast<gl::Context *>(getEGLWindow()->getContext());
+ gl::Program *glProgram = context->getProgram(prog.get());
+ GLint oneIndex = glProgram->getUniformLocation("ui_one");
+ ASSERT_NE(-1, oneIndex);
+ GLint twoIndex = glProgram->getUniformLocation("ui_two");
+ ASSERT_NE(-1, twoIndex);
+ GLint threeIndex = glProgram->getUniformLocation("ui_three");
+ ASSERT_NE(-1, threeIndex);
+ glUseProgram(prog.get());
+ glUniform1i(oneIndex, 1);
+ glUniform1i(twoIndex, 2);
+ glUniform1i(threeIndex, 3);
+
+ drawQuad(prog.get(), "position", 0.5f);
+ EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
+// Convers a bug with the unary minus operator on unsigned integer workaround.
+TEST_P(GLSLTest_ES3, UnaryMinusOperatorUnsignedInt)
+{
+ const std::string &vert =
+ "#version 300 es\n"
+ "in highp vec4 position;\n"
+ "out mediump vec4 v_color;\n"
+ "uniform uint ui_one;\n"
+ "uniform uint ui_two;\n"
+ "uniform uint ui_three;\n"
+ "void main() {\n"
+ " uint s[3];\n"
+ " s[0] = ui_one;\n"
+ " s[1] = -(-(-ui_two + 1u) + 1u);\n" // s[1] = -ui_two
+ " s[2] = ui_three;\n"
+ " uint result = 0u;\n"
+ " for (uint i = 0u; i < ui_three; i++) {\n"
+ " result += s[i];\n"
+ " }\n"
+ " v_color = (result == 2u) ? vec4(0, 1, 0, 1) : vec4(1, 0, 0, 1);\n"
+ " gl_Position = position;\n"
+ "}\n";
+ const std::string &frag =
+ "#version 300 es\n"
+ "in mediump vec4 v_color;\n"
+ "layout(location=0) out mediump vec4 o_color;\n"
+ "void main() {\n"
+ " o_color = v_color;\n"
+ "}\n";
+
+ ANGLE_GL_PROGRAM(prog, vert, frag);
+
+ gl::Context *context = reinterpret_cast<gl::Context *>(getEGLWindow()->getContext());
+ gl::Program *glProgram = context->getProgram(prog.get());
+ GLint oneIndex = glProgram->getUniformLocation("ui_one");
+ ASSERT_NE(-1, oneIndex);
+ GLint twoIndex = glProgram->getUniformLocation("ui_two");
+ ASSERT_NE(-1, twoIndex);
+ GLint threeIndex = glProgram->getUniformLocation("ui_three");
+ ASSERT_NE(-1, threeIndex);
+ glUseProgram(prog.get());
+ glUniform1ui(oneIndex, 1u);
+ glUniform1ui(twoIndex, 2u);
+ glUniform1ui(threeIndex, 3u);
+
+ drawQuad(prog.get(), "position", 0.5f);
+ EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
// Test a nested sequence operator with a ternary operator inside. The ternary operator is
// intended to be such that it gets converted to an if statement on the HLSL backend.
TEST_P(GLSLTest, NestedSequenceOperatorWithTernaryInside)