Fix break statement.
The break 'depth' was used to indicate the number of if/else execution
enable mask's stack levels we need to discard when jumping from a break
statement to its enclosing switch or loop. However, each switch and
loop resets this depth at the end, which isn't correct for nested loops
and/or switches (note that switches contain if/else statements, and
loops use the same 'enable' masks as if/else).
This can be fixed either by using a stack to keep track of the break
depths of nested switch/loop statements, or by simply not jumping
directly from the break statement to the end of it's enclosing switch
or loop. The latter fix was chosen for this change, which assumes that
that it's uncommon for all vector lanes to become disabled at the break
statement and skip many instructions. An important exception to this is
breaking out of an infinite or long-running loop, but this is handled
by checking the break enable mask as part of the loop condition.
Change-Id: I57d2e03941e855faefd997442931ff8619eca73f
Reviewed-on: https://swiftshader-review.googlesource.com/15968
Tested-by: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Alexis Hétu <sugoi@google.com>
Reviewed-by: Nicolas Capens <nicolascapens@google.com>
diff --git a/src/OpenGL/compiler/OutputASM.cpp b/src/OpenGL/compiler/OutputASM.cpp
index 2eb8329..b50a9b4 100644
--- a/src/OpenGL/compiler/OutputASM.cpp
+++ b/src/OpenGL/compiler/OutputASM.cpp
@@ -1772,10 +1772,16 @@
emit(sw::Shader::OPCODE_IF, 0, &result);
nbCases++;
+ // Emit the code for this case and all subsequent cases until we hit a break statement.
+ // TODO: This can repeat a lot of code for switches with many fall-through cases.
for(++caseIt; caseIt != sequence.end(); ++caseIt)
{
(*caseIt)->traverse(this);
- if((*caseIt)->getAsBranchNode()) // Kill, Break, Continue or Return
+
+ // Stop if we encounter an unconditional branch (break, continue, return, or kill).
+ // TODO: This doesn't work if the statement is at a deeper scope level (e.g. {break;}).
+ // Note that this eliminates useless operations but shouldn't affect correctness.
+ if((*caseIt)->getAsBranchNode())
{
break;
}
diff --git a/src/Shader/PixelProgram.cpp b/src/Shader/PixelProgram.cpp
index da7efdf..0f0f6bd 100644
--- a/src/Shader/PixelProgram.cpp
+++ b/src/Shader/PixelProgram.cpp
@@ -1251,25 +1251,7 @@
void PixelProgram::BREAK()
{
- BasicBlock *deadBlock = Nucleus::createBasicBlock();
- BasicBlock *endBlock = loopRepEndBlock[loopRepDepth - 1];
-
- if(breakDepth == 0)
- {
- enableIndex = enableIndex - breakDepth;
- Nucleus::createBr(endBlock);
- }
- else
- {
- enableBreak = enableBreak & ~enableStack[enableIndex];
- Bool allBreak = SignMask(enableBreak) == 0x0;
-
- enableIndex = enableIndex - breakDepth;
- branch(allBreak, endBlock, deadBlock);
- }
-
- Nucleus::setInsertBlock(deadBlock);
- enableIndex = enableIndex + breakDepth;
+ enableBreak = enableBreak & ~enableStack[enableIndex];
}
void PixelProgram::BREAKC(Vector4f &src0, Vector4f &src1, Control control)
@@ -1307,17 +1289,7 @@
{
condition &= enableStack[enableIndex];
- BasicBlock *continueBlock = Nucleus::createBasicBlock();
- BasicBlock *endBlock = loopRepEndBlock[loopRepDepth - 1];
-
enableBreak = enableBreak & ~condition;
- Bool allBreak = SignMask(enableBreak) == 0x0;
-
- enableIndex = enableIndex - breakDepth;
- branch(allBreak, endBlock, continueBlock);
-
- Nucleus::setInsertBlock(continueBlock);
- enableIndex = enableIndex + breakDepth;
}
void PixelProgram::CONTINUE()
@@ -1461,7 +1433,6 @@
if(isConditionalIf[ifDepth])
{
- breakDepth--;
enableIndex--;
}
}
@@ -1608,7 +1579,6 @@
ifFalseBlock[ifDepth] = falseBlock;
ifDepth++;
- breakDepth++;
}
void PixelProgram::LABEL(int labelIndex)
@@ -1652,7 +1622,6 @@
iteration[loopDepth] = iteration[loopDepth] - 1; // FIXME: --
loopRepDepth++;
- breakDepth = 0;
}
void PixelProgram::REP(const Src &integerRegister)
@@ -1679,7 +1648,6 @@
iteration[loopDepth] = iteration[loopDepth] - 1; // FIXME: --
loopRepDepth++;
- breakDepth = 0;
}
void PixelProgram::WHILE(const Src &temporaryRegister)
@@ -1705,6 +1673,7 @@
Int4 condition = As<Int4>(src.x);
condition &= enableStack[enableIndex - 1];
if(shader->containsLeaveInstruction()) condition &= enableLeave;
+ if(shader->containsBreakInstruction()) condition &= enableBreak;
enableStack[enableIndex] = condition;
Bool notAllFalse = SignMask(condition) != 0;
@@ -1716,7 +1685,6 @@
Nucleus::setInsertBlock(loopBlock);
loopRepDepth++;
- breakDepth = 0;
}
void PixelProgram::SWITCH()
@@ -1736,7 +1704,6 @@
Nucleus::setInsertBlock(currentBlock);
loopRepDepth++;
- breakDepth = 0;
}
void PixelProgram::RET()
diff --git a/src/Shader/PixelProgram.hpp b/src/Shader/PixelProgram.hpp
index 46af0ac..c66c863 100644
--- a/src/Shader/PixelProgram.hpp
+++ b/src/Shader/PixelProgram.hpp
@@ -25,7 +25,7 @@
public:
PixelProgram(const PixelProcessor::State &state, const PixelShader *shader) :
PixelRoutine(state, shader), r(shader && shader->dynamicallyIndexedTemporaries),
- loopDepth(-1), ifDepth(0), loopRepDepth(0), breakDepth(0), currentLabel(-1), whileTest(false)
+ loopDepth(-1), ifDepth(0), loopRepDepth(0), currentLabel(-1), whileTest(false)
{
for(int i = 0; i < 2048; ++i)
{
@@ -153,7 +153,6 @@
int ifDepth;
int loopRepDepth;
- int breakDepth;
int currentLabel;
bool whileTest;
diff --git a/src/Shader/VertexProgram.cpp b/src/Shader/VertexProgram.cpp
index 63b0edf..960772f 100644
--- a/src/Shader/VertexProgram.cpp
+++ b/src/Shader/VertexProgram.cpp
@@ -28,7 +28,6 @@
{
ifDepth = 0;
loopRepDepth = 0;
- breakDepth = 0;
currentLabel = -1;
whileTest = false;
@@ -1011,25 +1010,7 @@
void VertexProgram::BREAK()
{
- BasicBlock *deadBlock = Nucleus::createBasicBlock();
- BasicBlock *endBlock = loopRepEndBlock[loopRepDepth - 1];
-
- if(breakDepth == 0)
- {
- enableIndex = enableIndex - breakDepth;
- Nucleus::createBr(endBlock);
- }
- else
- {
- enableBreak = enableBreak & ~enableStack[enableIndex];
- Bool allBreak = SignMask(enableBreak) == 0x0;
-
- enableIndex = enableIndex - breakDepth;
- branch(allBreak, endBlock, deadBlock);
- }
-
- Nucleus::setInsertBlock(deadBlock);
- enableIndex = enableIndex + breakDepth;
+ enableBreak = enableBreak & ~enableStack[enableIndex];
}
void VertexProgram::BREAKC(Vector4f &src0, Vector4f &src1, Control control)
@@ -1067,17 +1048,7 @@
{
condition &= enableStack[enableIndex];
- BasicBlock *continueBlock = Nucleus::createBasicBlock();
- BasicBlock *endBlock = loopRepEndBlock[loopRepDepth - 1];
-
enableBreak = enableBreak & ~condition;
- Bool allBreak = SignMask(enableBreak) == 0x0;
-
- enableIndex = enableIndex - breakDepth;
- branch(allBreak, endBlock, continueBlock);
-
- Nucleus::setInsertBlock(continueBlock);
- enableIndex = enableIndex + breakDepth;
}
void VertexProgram::CONTINUE()
@@ -1221,7 +1192,6 @@
if(isConditionalIf[ifDepth])
{
- breakDepth--;
enableIndex--;
}
}
@@ -1368,7 +1338,6 @@
ifFalseBlock[ifDepth] = falseBlock;
ifDepth++;
- breakDepth++;
}
void VertexProgram::LABEL(int labelIndex)
@@ -1413,7 +1382,6 @@
iteration[loopDepth] = iteration[loopDepth] - 1; // FIXME: --
loopRepDepth++;
- breakDepth = 0;
}
void VertexProgram::REP(const Src &integerRegister)
@@ -1440,7 +1408,6 @@
iteration[loopDepth] = iteration[loopDepth] - 1; // FIXME: --
loopRepDepth++;
- breakDepth = 0;
}
void VertexProgram::WHILE(const Src &temporaryRegister)
@@ -1466,6 +1433,7 @@
Int4 condition = As<Int4>(src.x);
condition &= enableStack[enableIndex - 1];
if(shader->containsLeaveInstruction()) condition &= enableLeave;
+ if(shader->containsBreakInstruction()) condition &= enableBreak;
enableStack[enableIndex] = condition;
Bool notAllFalse = SignMask(condition) != 0;
@@ -1477,7 +1445,6 @@
Nucleus::setInsertBlock(loopBlock);
loopRepDepth++;
- breakDepth = 0;
}
void VertexProgram::SWITCH()
@@ -1497,7 +1464,6 @@
Nucleus::setInsertBlock(currentBlock);
loopRepDepth++;
- breakDepth = 0;
}
void VertexProgram::RET()
diff --git a/src/Shader/VertexProgram.hpp b/src/Shader/VertexProgram.hpp
index fddea49..c42ab71 100644
--- a/src/Shader/VertexProgram.hpp
+++ b/src/Shader/VertexProgram.hpp
@@ -122,7 +122,6 @@
int ifDepth;
int loopRepDepth;
- int breakDepth;
int currentLabel;
bool whileTest;
