Fix loop unrolling. Loops were no longer getting unrolled because we analyzed the entire loop, not just the body, for changes to the index variable. Also, the logic was inverted. Also, be conservative about loops with return statements not being unrollable. Bug chromium:845103 Change-Id: I5957e17f7b985ae90b10053216b6945f3f64338e Reviewed-on: https://swiftshader-review.googlesource.com/19008 Tested-by: Nicolas Capens <nicolascapens@google.com> Reviewed-by: Alexis Hétu <sugoi@google.com>
diff --git a/src/OpenGL/compiler/OutputASM.cpp b/src/OpenGL/compiler/OutputASM.cpp index d7213cb..8b20758 100644 --- a/src/OpenGL/compiler/OutputASM.cpp +++ b/src/OpenGL/compiler/OutputASM.cpp
@@ -1872,6 +1872,8 @@ if(unroll) { + mContext.info(node->getLine(), "loop unrolled", "for"); + for(unsigned int i = 0; i < loop.iterations; i++) { // condition->traverse(this); // Condition could contain statements, but not in an unrollable loop @@ -3886,32 +3888,21 @@ } } - bool LoopUnrollable::traverse(TIntermNode *node, int indexId) + bool LoopUnrollable::traverse(TIntermLoop *loop, int indexId) { loopUnrollable = true; - loopDepth = 0; loopIndexId = indexId; + TIntermNode *body = loop->getBody(); - node->traverse(this); + if(body) + { + body->traverse(this); + } return loopUnrollable; } - bool LoopUnrollable::visitLoop(Visit visit, TIntermLoop *loop) - { - if(visit == PreVisit) - { - loopDepth++; - } - else if(visit == PostVisit) - { - loopDepth++; - } - - return true; - } - void LoopUnrollable::visitSymbol(TIntermSymbol *node) { // Check that the loop index is not used as the argument to a function out or inout parameter. @@ -3933,7 +3924,7 @@ // Check that the loop index is not statically assigned to. TIntermSymbol *symbol = node->getLeft()->getAsSymbolNode(); - loopUnrollable = node->modifiesState() && symbol && (symbol->getId() == loopIndexId); + loopUnrollable = !(node->modifiesState() && symbol && (symbol->getId() == loopIndexId)); return loopUnrollable; } @@ -3947,7 +3938,7 @@ // Check that the loop index is not statically assigned to. TIntermSymbol *symbol = node->getOperand()->getAsSymbolNode(); - loopUnrollable = node->modifiesState() && symbol && (symbol->getId() == loopIndexId); + loopUnrollable = !(node->modifiesState() && symbol && (symbol->getId() == loopIndexId)); return loopUnrollable; } @@ -3959,16 +3950,10 @@ return false; } - if(!loopDepth) - { - return true; - } - switch(node->getFlowOp()) { case EOpKill: case EOpReturn: - break; case EOpBreak: case EOpContinue: loopUnrollable = false;
diff --git a/src/OpenGL/compiler/OutputASM.h b/src/OpenGL/compiler/OutputASM.h index 1aa791e..2f90266 100644 --- a/src/OpenGL/compiler/OutputASM.h +++ b/src/OpenGL/compiler/OutputASM.h
@@ -382,19 +382,17 @@ class LoopUnrollable : public TIntermTraverser { public: - bool traverse(TIntermNode *node, int loopIndexId); + bool traverse(TIntermLoop *loop, int loopIndexId); private: void visitSymbol(TIntermSymbol *node) override; bool visitBinary(Visit visit, TIntermBinary *node) override; bool visitUnary(Visit visit, TIntermUnary *node) override; bool visitBranch(Visit visit, TIntermBranch *node) override; - bool visitLoop(Visit visit, TIntermLoop *loop) override; bool visitAggregate(Visit visit, TIntermAggregate *node) override; bool loopUnrollable; - int loopDepth; int loopIndexId; }; }
diff --git a/tests/unittests/unittests.cpp b/tests/unittests/unittests.cpp index f263eda..afbb678 100644 --- a/tests/unittests/unittests.cpp +++ b/tests/unittests/unittests.cpp
@@ -24,6 +24,8 @@ #include <Windows.h> #endif +#include <string.h> + #define EXPECT_GLENUM_EQ(expected, actual) EXPECT_EQ(static_cast<GLenum>(expected), static_cast<GLenum>(actual)) class SwiftShaderTest : public testing::Test @@ -207,8 +209,8 @@ struct ProgramHandles { GLuint program; - GLuint vsShader; - GLuint fsShader; + GLuint vertexShader; + GLuint fragmentShader; }; ProgramHandles createProgram(const std::string& vs, const std::string& fs) @@ -217,20 +219,20 @@ ph.program = glCreateProgram(); EXPECT_GLENUM_EQ(GL_NONE, glGetError()); - ph.vsShader = glCreateShader(GL_VERTEX_SHADER); + ph.vertexShader = glCreateShader(GL_VERTEX_SHADER); const char* vsSource[1] = { vs.c_str() }; - glShaderSource(ph.vsShader, 1, vsSource, nullptr); - glCompileShader(ph.vsShader); + glShaderSource(ph.vertexShader, 1, vsSource, nullptr); + glCompileShader(ph.vertexShader); EXPECT_GLENUM_EQ(GL_NONE, glGetError()); - ph.fsShader = glCreateShader(GL_FRAGMENT_SHADER); + ph.fragmentShader = glCreateShader(GL_FRAGMENT_SHADER); const char* fsSource[1] = { fs.c_str() }; - glShaderSource(ph.fsShader, 1, fsSource, nullptr); - glCompileShader(ph.fsShader); + glShaderSource(ph.fragmentShader, 1, fsSource, nullptr); + glCompileShader(ph.fragmentShader); EXPECT_GLENUM_EQ(GL_NONE, glGetError()); - glAttachShader(ph.program, ph.vsShader); - glAttachShader(ph.program, ph.fsShader); + glAttachShader(ph.program, ph.vertexShader); + glAttachShader(ph.program, ph.fragmentShader); glLinkProgram(ph.program); EXPECT_GLENUM_EQ(GL_NONE, glGetError()); @@ -243,12 +245,12 @@ void deleteProgram(const ProgramHandles& ph) { - glDeleteShader(ph.fsShader); - glDeleteShader(ph.vsShader); + glDeleteShader(ph.fragmentShader); + glDeleteShader(ph.vertexShader); glDeleteProgram(ph.program); } - void drawQuad(GLuint program, const char* textureName) + void drawQuad(GLuint program, const char* textureName = nullptr) { GLint prevProgram = 0; glGetIntegerv(GL_CURRENT_PROGRAM, &prevProgram); @@ -315,6 +317,190 @@ Uninitialize(); } +// Test unrolling of a loop +TEST_F(SwiftShaderTest, UnrollLoop) +{ + Initialize(3, false); + + unsigned char green[4] = { 0, 255, 0, 255 }; + + const std::string vs = + "#version 300 es\n" + "in vec4 position;\n" + "out vec4 color;\n" + "void main()\n" + "{\n" + " for(int i = 0; i < 4; i++)\n" + " {\n" + " color[i] = (i % 2 == 0) ? 0.0 : 1.0;\n" + " }\n" + " gl_Position = vec4(position.xy, 0.0, 1.0);\n" + "}\n"; + + const std::string fs = + "#version 300 es\n" + "precision mediump float;\n" + "in vec4 color;\n" + "out vec4 fragColor;\n" + "void main()\n" + "{\n" + " fragColor = color;\n" + "}\n"; + + const ProgramHandles ph = createProgram(vs, fs); + + // Expect the info log to contain "unrolled". This is not a spec requirement. + GLsizei length = 0; + glGetShaderiv(ph.vertexShader, GL_INFO_LOG_LENGTH, &length); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + EXPECT_NE(length, 0); + char *log = new char[length]; + GLsizei written = 0; + glGetShaderInfoLog(ph.vertexShader, length, &written, log); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + EXPECT_EQ(length, written + 1); + EXPECT_NE(strstr(log, "unrolled"), nullptr); + delete log; + + glUseProgram(ph.program); + + glClearColor(0.0, 0.0, 0.0, 0.0); + glClear(GL_COLOR_BUFFER_BIT); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + drawQuad(ph.program); + + deleteProgram(ph); + + compareColor(green); + + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + Uninitialize(); +} + +// Test non-canonical or non-deterministic loops do not get unrolled +TEST_F(SwiftShaderTest, DynamicLoop) +{ + Initialize(3, false); + + unsigned char green[4] = { 0, 255, 0, 255 }; + + const std::string vs = + "#version 300 es\n" + "in vec4 position;\n" + "out vec4 color;\n" + "void main()\n" + "{\n" + " for(int i = 0; i < 4; )\n" + " {\n" + " color[i] = (i % 2 == 0) ? 0.0 : 1.0;\n" + " i++;" + " }\n" + " gl_Position = vec4(position.xy, 0.0, 1.0);\n" + "}\n"; + + const std::string fs = + "#version 300 es\n" + "precision mediump float;\n" + "in vec4 color;\n" + "out vec4 fragColor;\n" + "void main()\n" + "{\n" + " vec4 temp;" + " for(int i = 0; i < 4; i++)\n" + " {\n" + " if(color.x < 0.0) return;" + " temp[i] = color[i];\n" + " }\n" + " fragColor = vec4(temp[0], temp[1], temp[2], temp[3]);\n" + "}\n"; + + const ProgramHandles ph = createProgram(vs, fs); + + // Expect the info logs to be empty. This is not a spec requirement. + GLsizei length = 0; + glGetShaderiv(ph.vertexShader, GL_INFO_LOG_LENGTH, &length); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + EXPECT_EQ(length, 0); + glGetShaderiv(ph.fragmentShader, GL_INFO_LOG_LENGTH, &length); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + EXPECT_EQ(length, 0); + + glUseProgram(ph.program); + + glClearColor(0.0, 0.0, 0.0, 0.0); + glClear(GL_COLOR_BUFFER_BIT); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + drawQuad(ph.program); + + deleteProgram(ph); + + compareColor(green); + + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + Uninitialize(); +} + +// Test dynamic indexing +TEST_F(SwiftShaderTest, DynamicIndexing) +{ + Initialize(3, false); + + unsigned char green[4] = { 0, 255, 0, 255 }; + + const std::string vs = + "#version 300 es\n" + "in vec4 position;\n" + "out float color[4];\n" + "void main()\n" + "{\n" + " for(int i = 0; i < 4; )\n" + " {\n" + " int j = (gl_VertexID + i) % 4;\n" + " color[j] = (j % 2 == 0) ? 0.0 : 1.0;\n" + " i++;" + " }\n" + " gl_Position = vec4(position.xy, 0.0, 1.0);\n" + "}\n"; + + const std::string fs = + "#version 300 es\n" + "precision mediump float;\n" + "in float color[4];\n" + "out vec4 fragColor;\n" + "void main()\n" + "{\n" + " float temp[4];" + " for(int i = 0; i < 4; )\n" + " {\n" + " temp[i] = color[i];\n" + " i++;" + " }\n" + " fragColor = vec4(temp[0], temp[1], temp[2], temp[3]);\n" + "}\n"; + + const ProgramHandles ph = createProgram(vs, fs); + + glUseProgram(ph.program); + + glClearColor(0.0, 0.0, 0.0, 0.0); + glClear(GL_COLOR_BUFFER_BIT); + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + drawQuad(ph.program); + + deleteProgram(ph); + + compareColor(green); + + EXPECT_GLENUM_EQ(GL_NONE, glGetError()); + + Uninitialize(); +} + // Test sampling from a sampler in a struct as a function argument TEST_F(SwiftShaderTest, SamplerArrayInStructArrayAsFunctionArg) {