Disable optimizations for shaders with conditional discard in D3D9, and
only use expanded short-circuiting conditionals for expressions with
potential side-effects.
Conservatively assume aggreate and selection operators have side effects for now.
BUG=
ANGLEBUG=486
R=geofflang@chromium.org, kbr@chromium.org, nicolas@transgaming.com, shannonwoods@chromium.org
Review URL: https://codereview.appspot.com/14441075
Conflicts:
src/common/version.h
src/compiler/translator.vcxproj
src/compiler/translator.vcxproj.filters
src/compiler/translator/OutputHLSL.cpp
src/libGLESv2/ProgramBinary.cpp
src/libGLESv2/Shader.cpp
src/libGLESv2/Shader.h
Change-Id: Iaf9f10b5de7b33c927ef032f3c4fe9d5095f64dd
diff --git a/src/compiler/translator/Intermediate.cpp b/src/compiler/translator/Intermediate.cpp
index 7c8452c..803ffa6 100644
--- a/src/compiler/translator/Intermediate.cpp
+++ b/src/compiler/translator/Intermediate.cpp
@@ -774,7 +774,7 @@
//
// Returns true if state is modified.
//
-bool TIntermOperator::modifiesState() const
+bool TIntermOperator::hasSideEffects() const
{
switch (op) {
case EOpPostIncrement:
diff --git a/src/compiler/translator/NodeSearch.h b/src/compiler/translator/NodeSearch.h
new file mode 100644
index 0000000..27e471d
--- /dev/null
+++ b/src/compiler/translator/NodeSearch.h
@@ -0,0 +1,78 @@
+//
+// Copyright (c) 2002-2013 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.
+//
+// NodeSearch.h: Utilities for searching translator node graphs
+//
+
+#ifndef TRANSLATOR_NODESEARCH_H_
+#define TRANSLATOR_NODESEARCH_H_
+
+namespace sh
+{
+
+template <class Parent>
+class NodeSearchTraverser : public TIntermTraverser
+{
+ public:
+ NodeSearchTraverser()
+ : mFound(false)
+ {}
+
+ bool found() const { return mFound; }
+
+ static bool search(TIntermNode *node)
+ {
+ Parent searchTraverser;
+ node->traverse(&searchTraverser);
+ return searchTraverser.found();
+ }
+
+ protected:
+ bool mFound;
+};
+
+class FindDiscard : public NodeSearchTraverser<FindDiscard>
+{
+ public:
+ virtual bool visitBranch(Visit visit, TIntermBranch *node)
+ {
+ switch (node->getFlowOp())
+ {
+ case EOpKill:
+ mFound = true;
+ break;
+
+ default: break;
+ }
+
+ return !mFound;
+ }
+};
+
+class FindSideEffectRewriting : public NodeSearchTraverser<FindSideEffectRewriting>
+{
+ public:
+ virtual bool visitBinary(Visit visit, TIntermBinary *node)
+ {
+ switch (node->getOp())
+ {
+ case EOpLogicalOr:
+ case EOpLogicalAnd:
+ if (node->getRight()->hasSideEffects())
+ {
+ mFound = true;
+ }
+ break;
+
+ default: break;
+ }
+
+ return !mFound;
+ }
+};
+
+}
+
+#endif // TRANSLATOR_NODESEARCH_H_
diff --git a/src/compiler/translator/OutputHLSL.cpp b/src/compiler/translator/OutputHLSL.cpp
index c410ba2..7009390 100644
--- a/src/compiler/translator/OutputHLSL.cpp
+++ b/src/compiler/translator/OutputHLSL.cpp
@@ -15,6 +15,7 @@
#include "compiler/translator/UnfoldShortCircuit.h"
#include "compiler/translator/HLSLLayoutEncoder.h"
#include "compiler/translator/FlagStd140Structs.h"
+#include "compiler/translator/NodeSearch.h"
#include <algorithm>
#include <cfloat>
@@ -121,6 +122,7 @@
mUsesAtan2_2 = false;
mUsesAtan2_3 = false;
mUsesAtan2_4 = false;
+ mUsesDiscardRewriting = false;
mNumRenderTargets = resources.EXT_draw_buffers ? resources.MaxDrawBuffers : 1;
@@ -663,6 +665,11 @@
out << *constructor;
}
+ if (mUsesDiscardRewriting)
+ {
+ out << "#define ANGLE_USES_DISCARD_REWRITING" << "\n";
+ }
+
if (mContext.shaderType == SH_FRAGMENT_SHADER)
{
TExtensionBehavior::const_iterator iter = mContext.extensionBehavior().find("GL_EXT_draw_buffers");
@@ -1883,15 +1890,31 @@
case EOpMatrixTimesVector: outputTriplet(visit, "mul(transpose(", "), ", ")"); break;
case EOpMatrixTimesMatrix: outputTriplet(visit, "transpose(mul(transpose(", "), transpose(", ")))"); break;
case EOpLogicalOr:
- out << "s" << mUnfoldShortCircuit->getNextTemporaryIndex();
- return false;
+ if (node->getRight()->hasSideEffects())
+ {
+ out << "s" << mUnfoldShortCircuit->getNextTemporaryIndex();
+ return false;
+ }
+ else
+ {
+ outputTriplet(visit, "(", " || ", ")");
+ return true;
+ }
case EOpLogicalXor:
mUsesXor = true;
outputTriplet(visit, "xor(", ", ", ")");
break;
case EOpLogicalAnd:
- out << "s" << mUnfoldShortCircuit->getNextTemporaryIndex();
- return false;
+ if (node->getRight()->hasSideEffects())
+ {
+ out << "s" << mUnfoldShortCircuit->getNextTemporaryIndex();
+ return false;
+ }
+ else
+ {
+ outputTriplet(visit, "(", " && ", ")");
+ return true;
+ }
default: UNREACHABLE();
}
@@ -2483,7 +2506,7 @@
{
mUnfoldShortCircuit->traverse(node->getCondition());
- out << "if(";
+ out << "if (";
node->getCondition()->traverse(this);
@@ -2492,9 +2515,14 @@
outputLineDirective(node->getLine().first_line);
out << "{\n";
+ bool discard = false;
+
if (node->getTrueBlock())
{
traverseStatements(node->getTrueBlock());
+
+ // Detect true discard
+ discard = (discard || FindDiscard::search(node->getTrueBlock()));
}
outputLineDirective(node->getLine().first_line);
@@ -2512,6 +2540,15 @@
outputLineDirective(node->getFalseBlock()->getLine().first_line);
out << ";\n}\n";
+
+ // Detect false discard
+ discard = (discard || FindDiscard::search(node->getFalseBlock()));
+ }
+
+ // ANGLE issue 486: Detect problematic conditional discard
+ if (discard && FindSideEffectRewriting::search(node))
+ {
+ mUsesDiscardRewriting = true;
}
}
@@ -2609,7 +2646,9 @@
switch (node->getFlowOp())
{
- case EOpKill: outputTriplet(visit, "discard;\n", "", ""); break;
+ case EOpKill:
+ outputTriplet(visit, "discard;\n", "", "");
+ break;
case EOpBreak:
if (visit == PreVisit)
{
@@ -2832,7 +2871,7 @@
if (!firstLoopFragment)
{
- out << "if(!Break";
+ out << "if (!Break";
index->traverse(this);
out << ") {\n";
}
diff --git a/src/compiler/translator/OutputHLSL.h b/src/compiler/translator/OutputHLSL.h
index 0392b0d..728682e 100644
--- a/src/compiler/translator/OutputHLSL.h
+++ b/src/compiler/translator/OutputHLSL.h
@@ -145,6 +145,7 @@
bool mUsesAtan2_2;
bool mUsesAtan2_3;
bool mUsesAtan2_4;
+ bool mUsesDiscardRewriting;
int mNumRenderTargets;
diff --git a/src/compiler/translator/UnfoldShortCircuit.cpp b/src/compiler/translator/UnfoldShortCircuit.cpp
index 303a192..b782611 100644
--- a/src/compiler/translator/UnfoldShortCircuit.cpp
+++ b/src/compiler/translator/UnfoldShortCircuit.cpp
@@ -31,6 +31,14 @@
{
TInfoSinkBase &out = mOutputHLSL->getBodyStream();
+ // If our right node doesn't have side effects, we know we don't need to unfold this
+ // expression: there will be no short-circuiting side effects to avoid
+ // (note: unfolding doesn't depend on the left node -- it will always be evaluated)
+ if (!node->getRight()->hasSideEffects())
+ {
+ return true;
+ }
+
switch (node->getOp())
{
case EOpLogicalOr:
@@ -49,7 +57,7 @@
mTemporaryIndex = i + 1;
node->getLeft()->traverse(mOutputHLSL);
out << ";\n";
- out << "if(!s" << i << ")\n"
+ out << "if (!s" << i << ")\n"
"{\n";
mTemporaryIndex = i + 1;
node->getRight()->traverse(this);
@@ -80,7 +88,7 @@
mTemporaryIndex = i + 1;
node->getLeft()->traverse(mOutputHLSL);
out << ";\n";
- out << "if(s" << i << ")\n"
+ out << "if (s" << i << ")\n"
"{\n";
mTemporaryIndex = i + 1;
node->getRight()->traverse(this);
@@ -115,7 +123,7 @@
mTemporaryIndex = i + 1;
node->getCondition()->traverse(this);
- out << "if(";
+ out << "if (";
mTemporaryIndex = i + 1;
node->getCondition()->traverse(mOutputHLSL);
out << ")\n"
diff --git a/src/compiler/translator/ValidateLimitations.cpp b/src/compiler/translator/ValidateLimitations.cpp
index 94d3994..f06a96f 100644
--- a/src/compiler/translator/ValidateLimitations.cpp
+++ b/src/compiler/translator/ValidateLimitations.cpp
@@ -461,7 +461,7 @@
bool ValidateLimitations::validateOperation(TIntermOperator* node,
TIntermNode* operand) {
// Check if loop index is modified in the loop body.
- if (!withinLoopBody() || !node->modifiesState())
+ if (!withinLoopBody() || !node->hasSideEffects())
return true;
const TIntermSymbol* symbol = operand->getAsSymbolNode();
diff --git a/src/compiler/translator/depgraph/DependencyGraphBuilder.cpp b/src/compiler/translator/depgraph/DependencyGraphBuilder.cpp
index 79f094c..ad8ec0e 100644
--- a/src/compiler/translator/depgraph/DependencyGraphBuilder.cpp
+++ b/src/compiler/translator/depgraph/DependencyGraphBuilder.cpp
@@ -94,7 +94,7 @@
bool TDependencyGraphBuilder::visitBinary(Visit visit, TIntermBinary* intermBinary)
{
TOperator op = intermBinary->getOp();
- if (op == EOpInitialize || intermBinary->modifiesState())
+ if (op == EOpInitialize || intermBinary->hasSideEffects())
visitAssignment(intermBinary);
else if (op == EOpLogicalAnd || op == EOpLogicalOr)
visitLogicalOp(intermBinary);
diff --git a/src/compiler/translator/intermediate.h b/src/compiler/translator/intermediate.h
index c286149..f252298 100644
--- a/src/compiler/translator/intermediate.h
+++ b/src/compiler/translator/intermediate.h
@@ -258,6 +258,8 @@
TIntermTyped(const TType& t) : type(t) { }
virtual TIntermTyped* getAsTyped() { return this; }
+ virtual bool hasSideEffects() const = 0;
+
void setType(const TType& t) { type = t; }
const TType& getType() const { return type; }
TType* getTypePointer() { return &type; }
@@ -359,6 +361,8 @@
TIntermSymbol(int i, const TString& sym, const TType& t) :
TIntermTyped(t), id(i) { symbol = sym; originalSymbol = sym; }
+ virtual bool hasSideEffects() const { return false; }
+
int getId() const { return id; }
const TString& getSymbol() const { return symbol; }
@@ -380,6 +384,8 @@
public:
TIntermConstantUnion(ConstantUnion *unionPointer, const TType& t) : TIntermTyped(t), unionArrayPointer(unionPointer) { }
+ virtual bool hasSideEffects() const { return false; }
+
ConstantUnion* getUnionArrayPointer() const { return unionArrayPointer; }
int getIConst(size_t index) const { return unionArrayPointer ? unionArrayPointer[index].getIConst() : 0; }
@@ -404,7 +410,7 @@
TOperator getOp() const { return op; }
void setOp(TOperator o) { op = o; }
- bool modifiesState() const;
+ virtual bool hasSideEffects() const;
bool isConstructor() const;
protected:
@@ -423,6 +429,8 @@
virtual TIntermBinary* getAsBinaryNode() { return this; }
virtual void traverse(TIntermTraverser*);
+ virtual bool hasSideEffects() const { return (TIntermOperator::hasSideEffects() || left->hasSideEffects() || right->hasSideEffects()); }
+
void setLeft(TIntermTyped* n) { left = n; }
void setRight(TIntermTyped* n) { right = n; }
TIntermTyped* getLeft() const { return left; }
@@ -451,6 +459,8 @@
virtual void traverse(TIntermTraverser*);
virtual TIntermUnary* getAsUnaryNode() { return this; }
+ virtual bool hasSideEffects() const { return (TIntermOperator::hasSideEffects() || operand->hasSideEffects()); }
+
void setOperand(TIntermTyped* o) { operand = o; }
TIntermTyped* getOperand() { return operand; }
bool promote(TInfoSink&);
@@ -481,6 +491,9 @@
virtual TIntermAggregate* getAsAggregate() { return this; }
virtual void traverse(TIntermTraverser*);
+ // Conservatively assume function calls and other aggregate operators have side-effects
+ virtual bool hasSideEffects() const { return true; }
+
TIntermSequence& getSequence() { return sequence; }
void setName(const TString& n) { name = n; }
@@ -524,6 +537,9 @@
virtual void traverse(TIntermTraverser*);
+ // Conservatively assume selections have side-effects
+ virtual bool hasSideEffects() const { return true; }
+
bool usesTernaryOperator() const { return getBasicType() != EbtVoid; }
TIntermNode* getCondition() const { return condition; }
TIntermNode* getTrueBlock() const { return trueBlock; }