Skip to content

A
angle
Commit 914b79a6 by Olli Etuaho Committed by Commit Bot
Options

Fix parsing GLSL loop conditions that declare a variable

Now the variable declaration is included in the AST, so that the loop body may refer to the variable. The variable declaration is placed in a block that wraps the loop. This way we can still only have TIntermTyped loop conditions in the AST, which keeps the code dealing with loops fairly simple and type safe. This change includes reversing the return value of executeInitializer, so that it returns true on success and false on error. This is more in line with other ParseContext member functions. BUG=angleproject:2073 TEST=angle_end2end_tests Change-Id: I5c4ecbf1b438d3fff6d6237c0dcf191e2a19664c Reviewed-on: https://chromium-review.googlesource.com/539639Reviewed-by: 's avatarCorentin Wallez <cwallez@chromium.org> Reviewed-by: 's avatarJamie Madill <jmadill@chromium.org> Commit-Queue: Olli Etuaho <oetuaho@nvidia.com>
parent 8700a98e
Showing with 175 additions and 99 deletions
src/compiler/translator/Intermediate.cpp
......@@ -226,22 +226,6 @@ TIntermTyped *TIntermediate::AddSwizzle(TIntermTyped *baseExpression,
}
//
// Create loop nodes.
//
TIntermNode *TIntermediate::addLoop(TLoopType type,
TIntermNode *init,
TIntermTyped *cond,
TIntermTyped *expr,
TIntermNode *body,
const TSourceLoc &line)
{
TIntermNode *node = new TIntermLoop(type, init, cond, expr, EnsureBlock(body));
node->setLine(line);
return node;
}
//
// Add branches.
//
TIntermBranch *TIntermediate::addBranch(TOperator branchOp, const TSourceLoc &line)
......
src/compiler/translator/Intermediate.h
......@@ -50,12 +50,7 @@ class TIntermediate
TIntermConstantUnion *addConstantUnion(const TConstantUnion *constantUnion,
const TType &type,
const TSourceLoc &line);
TIntermNode *addLoop(TLoopType,
TIntermNode *,
TIntermTyped *,
TIntermTyped *,
TIntermNode *,
const TSourceLoc &);
TIntermBranch *addBranch(TOperator, const TSourceLoc &);
TIntermBranch *addBranch(TOperator, TIntermTyped *, const TSourceLoc &);
static TIntermTyped *AddSwizzle(TIntermTyped *baseExpression,
......
src/compiler/translator/ParseContext.cpp
......@@ -1673,12 +1673,10 @@ TIntermTyped *TParseContext::parseVariableIdentifier(const TSourceLoc &location,
}
}
//
// Initializers show up in several places in the grammar. Have one set of
// code to handle them here.
//
// Returns true on error, false if no error
//
// Returns true on success.
bool TParseContext::executeInitializer(const TSourceLoc &line,
const TString &identifier,
const TPublicType &pType,
......@@ -1706,7 +1704,7 @@ bool TParseContext::executeInitializer(const TSourceLoc &line,
}
if (!declareVariable(line, identifier, type, &variable))
{
return true;
return false;
}
bool globalInitWarning = false;
......@@ -1716,7 +1714,7 @@ bool TParseContext::executeInitializer(const TSourceLoc &line,
// Error message does not completely match behavior with ESSL 1.00, but
// we want to steer developers towards only using constant expressions.
error(line, "global variable initializers must be constant expressions", "=");
return true;
return false;
}
if (globalInitWarning)
{
......@@ -1735,7 +1733,7 @@ bool TParseContext::executeInitializer(const TSourceLoc &line,
{
error(line, " cannot initialize this type of qualifier ",
variable->getType().getQualifierString());
return true;
return false;
}
//
// test for and propagate constant
......@@ -1751,14 +1749,14 @@ bool TParseContext::executeInitializer(const TSourceLoc &line,
std::string reason = reasonStream.str();
error(line, reason.c_str(), "=");
variable->getType().setQualifier(EvqTemporary);
return true;
return false;
}
if (type != initializer->getType())
{
error(line, " non-matching types for const initializer ",
variable->getType().getQualifierString());
variable->getType().setQualifier(EvqTemporary);
return true;
return false;
}
// Save the constant folded value to the variable if possible. For example array
......@@ -1769,8 +1767,8 @@ bool TParseContext::executeInitializer(const TSourceLoc &line,
if (initializer->getAsConstantUnion())
{
variable->shareConstPointer(initializer->getAsConstantUnion()->getUnionArrayPointer());
*initNode = nullptr;
return false;
ASSERT(*initNode == nullptr);
return true;
}
else if (initializer->getAsSymbolNode())
{
......@@ -1782,8 +1780,8 @@ bool TParseContext::executeInitializer(const TSourceLoc &line,
if (constArray)
{
variable->shareConstPointer(constArray);
*initNode = nullptr;
return false;
ASSERT(*initNode == nullptr);
return true;
}
}
}
......@@ -1794,10 +1792,80 @@ bool TParseContext::executeInitializer(const TSourceLoc &line,
if (*initNode == nullptr)
{
assignError(line, "=", intermSymbol->getCompleteString(), initializer->getCompleteString());
return true;
return false;
}
return false;
return true;
}
TIntermNode *TParseContext::addConditionInitializer(const TPublicType &pType,
const TString &identifier,
TIntermTyped *initializer,
const TSourceLoc &loc)
{
checkIsScalarBool(loc, pType);
TIntermBinary *initNode = nullptr;
if (executeInitializer(loc, identifier, pType, initializer, &initNode))
{
// The initializer is valid. The init condition needs to have a node - either the
// initializer node, or a constant node in case the initialized variable is const and won't
// be recorded in the AST.
if (initNode == nullptr)
{
return initializer;
}
else
{
TIntermDeclaration *declaration = new TIntermDeclaration();
declaration->appendDeclarator(initNode);
return declaration;
}
}
return nullptr;
}
TIntermNode *TParseContext::addLoop(TLoopType type,
TIntermNode *init,
TIntermNode *cond,
TIntermTyped *expr,
TIntermNode *body,
const TSourceLoc &line)
{
TIntermNode *node = nullptr;
TIntermTyped *typedCond = nullptr;
if (cond)
{
typedCond = cond->getAsTyped();
}
if (cond == nullptr || typedCond)
{
node = new TIntermLoop(type, init, typedCond, expr, TIntermediate::EnsureBlock(body));
node->setLine(line);
return node;
}
TIntermDeclaration *declaration = cond->getAsDeclarationNode();
ASSERT(declaration);
TIntermBinary *declarator = declaration->getSequence()->front()->getAsBinaryNode();
ASSERT(declarator->getLeft()->getAsSymbolNode());
// The condition is a declaration. In the AST representation we don't support declarations as
// loop conditions. Wrap the loop to a block that declares the condition variable and contains
// the loop.
TIntermBlock *block = new TIntermBlock();
TIntermDeclaration *declareCondition = new TIntermDeclaration();
declareCondition->appendDeclarator(declarator->getLeft()->deepCopy());
block->appendStatement(declareCondition);
TIntermBinary *conditionInit = new TIntermBinary(EOpAssign, declarator->getLeft()->deepCopy(),
declarator->getRight()->deepCopy());
TIntermLoop *loop =
new TIntermLoop(type, init, conditionInit, expr, TIntermediate::EnsureBlock(body));
block->appendStatement(loop);
loop->setLine(line);
block->setLine(line);
return block;
}
void TParseContext::addFullySpecifiedType(TPublicType *typeSpecifier)
......@@ -2171,7 +2239,7 @@ TIntermDeclaration *TParseContext::parseSingleInitDeclaration(const TPublicType
declaration->setLine(identifierLocation);
TIntermBinary *initNode = nullptr;
if (!executeInitializer(identifierLocation, identifier, publicType, initializer, &initNode))
if (executeInitializer(identifierLocation, identifier, publicType, initializer, &initNode))
{
if (initNode)
{
......@@ -2217,7 +2285,7 @@ TIntermDeclaration *TParseContext::parseSingleArrayInitDeclaration(
// initNode will correspond to the whole of "type b[n] = initializer".
TIntermBinary *initNode = nullptr;
if (!executeInitializer(identifierLocation, identifier, arrayType, initializer, &initNode))
if (executeInitializer(identifierLocation, identifier, arrayType, initializer, &initNode))
{
if (initNode)
{
......@@ -2376,7 +2444,7 @@ void TParseContext::parseInitDeclarator(const TPublicType &publicType,
checkDeclaratorLocationIsNotSpecified(identifierLocation, publicType);
TIntermBinary *initNode = nullptr;
if (!executeInitializer(identifierLocation, identifier, publicType, initializer, &initNode))
if (executeInitializer(identifierLocation, identifier, publicType, initializer, &initNode))
{
//
// build the intermediate representation
......@@ -2424,7 +2492,7 @@ void TParseContext::parseArrayInitDeclarator(const TPublicType &publicType,
// initNode will correspond to the whole of "b[n] = initializer".
TIntermBinary *initNode = nullptr;
if (!executeInitializer(identifierLocation, identifier, arrayType, initializer, &initNode))
if (executeInitializer(identifierLocation, identifier, arrayType, initializer, &initNode))
{
if (initNode)
{
......
src/compiler/translator/ParseContext.h
......@@ -175,11 +175,23 @@ class TParseContext : angle::NonCopyable
const char *value,
bool stdgl);
// Returns true on success. *initNode may still be nullptr on success in case the initialization
// is not needed in the AST.
bool executeInitializer(const TSourceLoc &line,
const TString &identifier,
const TPublicType &pType,
TIntermTyped *initializer,
TIntermBinary **initNode);
TIntermNode *addConditionInitializer(const TPublicType &pType,
const TString &identifier,
TIntermTyped *initializer,
const TSourceLoc &loc);
TIntermNode *addLoop(TLoopType type,
TIntermNode *init,
TIntermNode *cond,
TIntermTyped *expr,
TIntermNode *body,
const TSourceLoc &loc);
void addFullySpecifiedType(TPublicType *typeSpecifier);
TPublicType addFullySpecifiedType(const TTypeQualifierBuilder &typeQualifierBuilder,
......
src/compiler/translator/glslang.y
......@@ -212,8 +212,9 @@ extern void yyerror(YYLTYPE* yylloc, TParseContext* context, void *scanner, cons
%type <interm.intermTypedNode> conditional_expression constant_expression
%type <interm.intermTypedNode> logical_or_expression logical_xor_expression logical_and_expression
%type <interm.intermTypedNode> shift_expression and_expression exclusive_or_expression inclusive_or_expression
%type <interm.intermTypedNode> function_call initializer condition conditionopt
%type <interm.intermTypedNode> function_call initializer
%type <interm.intermNode> condition conditionopt
%type <interm.intermBlock> translation_unit
%type <interm.intermNode> function_definition statement simple_statement
%type <interm.intermBlock> statement_list compound_statement compound_statement_no_new_scope
......@@ -1451,32 +1452,25 @@ condition
context->checkIsScalarBool($1->getLine(), $1);
}
| fully_specified_type identifier EQUAL initializer {
TIntermBinary *initNode = nullptr;
context->checkIsScalarBool(@2, $1);
if (!context->executeInitializer(@2, *$2.string, $1, $4, &initNode))
$$ = $4;
else {
$$ = 0;
}
$$ = context->addConditionInitializer($1, *$2.string, $4, @2);
}
;
iteration_statement
: WHILE LEFT_PAREN { context->symbolTable.push(); context->incrLoopNestingLevel(); } condition RIGHT_PAREN statement_no_new_scope {
context->symbolTable.pop();
$$ = context->intermediate.addLoop(ELoopWhile, 0, $4, 0, $6, @1);
$$ = context->addLoop(ELoopWhile, 0, $4, 0, $6, @1);
context->decrLoopNestingLevel();
}
| DO { context->incrLoopNestingLevel(); } statement_with_scope WHILE LEFT_PAREN expression RIGHT_PAREN SEMICOLON {
context->checkIsScalarBool(@8, $6);
$$ = context->intermediate.addLoop(ELoopDoWhile, 0, $6, 0, $3, @4);
$$ = context->addLoop(ELoopDoWhile, 0, $6, 0, $3, @4);
context->decrLoopNestingLevel();
}
| FOR LEFT_PAREN { context->symbolTable.push(); context->incrLoopNestingLevel(); } for_init_statement for_rest_statement RIGHT_PAREN statement_no_new_scope {
context->symbolTable.pop();
$$ = context->intermediate.addLoop(ELoopFor, $4, reinterpret_cast<TIntermTyped*>($5.node1), reinterpret_cast<TIntermTyped*>($5.node2), $7, @1);
$$ = context->addLoop(ELoopFor, $4, reinterpret_cast<TIntermTyped*>($5.node1), reinterpret_cast<TIntermTyped*>($5.node2), $7, @1);
context->decrLoopNestingLevel();
}
;
......@@ -1495,7 +1489,7 @@ conditionopt
$$ = $1;
}
| /* May be null */ {
$$ = 0;
$$ = nullptr;
}
;
......
src/compiler/translator/glslang_tab.cpp
......@@ -752,36 +752,36 @@ static const yytype_uint8 yytranslate[] =
/* YYRLINE[YYN] -- Source line where rule number YYN was defined. */
static const yytype_uint16 yyrline[] =
{
0, 255, 255, 256, 259, 269, 272, 277, 282, 287,
292, 300, 306, 309, 312, 315, 318, 321, 327, 334,
340, 344, 352, 355, 361, 365, 372, 377, 384, 392,
398, 404, 413, 416, 419, 422, 432, 433, 434, 435,
443, 444, 447, 450, 457, 458, 461, 467, 468, 472,
479, 480, 483, 486, 489, 495, 496, 499, 505, 506,
513, 514, 521, 522, 529, 530, 536, 537, 543, 544,
550, 551, 557, 558, 565, 566, 567, 568, 572, 573,
574, 578, 582, 586, 590, 597, 600, 606, 613, 620,
623, 626, 635, 639, 643, 647, 651, 658, 665, 668,
675, 683, 703, 713, 721, 746, 750, 754, 758, 765,
772, 775, 779, 783, 788, 793, 800, 804, 808, 812,
817, 822, 829, 833, 839, 842, 848, 852, 859, 865,
869, 873, 876, 879, 888, 894, 902, 905, 925, 944,
951, 955, 959, 962, 965, 968, 971, 974, 982, 989,
992, 995, 1001, 1008, 1011, 1017, 1020, 1023, 1026, 1032,
1035, 1040, 1051, 1054, 1057, 1060, 1063, 1066, 1070, 1074,
1078, 1082, 1086, 1090, 1094, 1098, 1102, 1106, 1110, 1114,
1118, 1122, 1126, 1130, 1134, 1138, 1142, 1146, 1150, 1156,
1159, 1162, 1165, 1168, 1171, 1174, 1177, 1180, 1183, 1186,
1189, 1192, 1195, 1198, 1201, 1204, 1207, 1210, 1217, 1223,
1229, 1232, 1235, 1238, 1241, 1244, 1247, 1250, 1253, 1256,
1259, 1262, 1265, 1268, 1271, 1283, 1283, 1286, 1286, 1292,
1295, 1301, 1304, 1311, 1315, 1321, 1327, 1339, 1343, 1347,
1348, 1354, 1355, 1356, 1357, 1358, 1359, 1360, 1364, 1365,
1365, 1365, 1374, 1375, 1379, 1379, 1380, 1380, 1385, 1388,
1397, 1402, 1409, 1410, 1414, 1421, 1425, 1432, 1432, 1439,
1442, 1449, 1453, 1466, 1466, 1471, 1471, 1477, 1477, 1485,
1488, 1494, 1497, 1503, 1507, 1514, 1517, 1520, 1523, 1526,
1535, 1541, 1547, 1550, 1556, 1556
0, 256, 256, 257, 260, 270, 273, 278, 283, 288,
293, 301, 307, 310, 313, 316, 319, 322, 328, 335,
341, 345, 353, 356, 362, 366, 373, 378, 385, 393,
399, 405, 414, 417, 420, 423, 433, 434, 435, 436,
444, 445, 448, 451, 458, 459, 462, 468, 469, 473,
480, 481, 484, 487, 490, 496, 497, 500, 506, 507,
514, 515, 522, 523, 530, 531, 537, 538, 544, 545,
551, 552, 558, 559, 566, 567, 568, 569, 573, 574,
575, 579, 583, 587, 591, 598, 601, 607, 614, 621,
624, 627, 636, 640, 644, 648, 652, 659, 666, 669,
676, 684, 704, 714, 722, 747, 751, 755, 759, 766,
773, 776, 780, 784, 789, 794, 801, 805, 809, 813,
818, 823, 830, 834, 840, 843, 849, 853, 860, 866,
870, 874, 877, 880, 889, 895, 903, 906, 926, 945,
952, 956, 960, 963, 966, 969, 972, 975, 983, 990,
993, 996, 1002, 1009, 1012, 1018, 1021, 1024, 1027, 1033,
1036, 1041, 1052, 1055, 1058, 1061, 1064, 1067, 1071, 1075,
1079, 1083, 1087, 1091, 1095, 1099, 1103, 1107, 1111, 1115,
1119, 1123, 1127, 1131, 1135, 1139, 1143, 1147, 1151, 1157,
1160, 1163, 1166, 1169, 1172, 1175, 1178, 1181, 1184, 1187,
1190, 1193, 1196, 1199, 1202, 1205, 1208, 1211, 1218, 1224,
1230, 1233, 1236, 1239, 1242, 1245, 1248, 1251, 1254, 1257,
1260, 1263, 1266, 1269, 1272, 1284, 1284, 1287, 1287, 1293,
1296, 1302, 1305, 1312, 1316, 1322, 1328, 1340, 1344, 1348,
1349, 1355, 1356, 1357, 1358, 1359, 1360, 1361, 1365, 1366,
1366, 1366, 1375, 1376, 1380, 1380, 1381, 1381, 1386, 1389,
1398, 1403, 1410, 1411, 1415, 1422, 1426, 1433, 1433, 1440,
1443, 1450, 1454, 1460, 1460, 1465, 1465, 1471, 1471, 1479,
1482, 1488, 1491, 1497, 1501, 1508, 1511, 1514, 1517, 1520,
1529, 1535, 1541, 1544, 1550, 1550
};
#endif
......@@ -4828,7 +4828,7 @@ yyreduce:
case 271:
{
(yyval.interm.intermTypedNode) = (yyvsp[0].interm.intermTypedNode);
(yyval.interm.intermNode) = (yyvsp[0].interm.intermTypedNode);
context->checkIsScalarBool((yyvsp[0].interm.intermTypedNode)->getLine(), (yyvsp[0].interm.intermTypedNode));
}
......@@ -4837,14 +4837,7 @@ yyreduce:
case 272:
{
TIntermBinary *initNode = nullptr;
context->checkIsScalarBool((yylsp[-2]), (yyvsp[-3].interm.type));
if (!context->executeInitializer((yylsp[-2]), *(yyvsp[-2].lex).string, (yyvsp[-3].interm.type), (yyvsp[0].interm.intermTypedNode), &initNode))
(yyval.interm.intermTypedNode) = (yyvsp[0].interm.intermTypedNode);
else {
(yyval.interm.intermTypedNode) = 0;
}
(yyval.interm.intermNode) = context->addConditionInitializer((yyvsp[-3].interm.type), *(yyvsp[-2].lex).string, (yyvsp[0].interm.intermTypedNode), (yylsp[-2]));
}
break;
......@@ -4859,7 +4852,7 @@ yyreduce:
{
context->symbolTable.pop();
(yyval.interm.intermNode) = context->intermediate.addLoop(ELoopWhile, 0, (yyvsp[-2].interm.intermTypedNode), 0, (yyvsp[0].interm.intermNode), (yylsp[-5]));
(yyval.interm.intermNode) = context->addLoop(ELoopWhile, 0, (yyvsp[-2].interm.intermNode), 0, (yyvsp[0].interm.intermNode), (yylsp[-5]));
context->decrLoopNestingLevel();
}
......@@ -4876,7 +4869,7 @@ yyreduce:
{
context->checkIsScalarBool((yylsp[0]), (yyvsp[-2].interm.intermTypedNode));
(yyval.interm.intermNode) = context->intermediate.addLoop(ELoopDoWhile, 0, (yyvsp[-2].interm.intermTypedNode), 0, (yyvsp[-5].interm.intermNode), (yylsp[-4]));
(yyval.interm.intermNode) = context->addLoop(ELoopDoWhile, 0, (yyvsp[-2].interm.intermTypedNode), 0, (yyvsp[-5].interm.intermNode), (yylsp[-4]));
context->decrLoopNestingLevel();
}
......@@ -4892,7 +4885,7 @@ yyreduce:
{
context->symbolTable.pop();
(yyval.interm.intermNode) = context->intermediate.addLoop(ELoopFor, (yyvsp[-3].interm.intermNode), reinterpret_cast<TIntermTyped*>((yyvsp[-2].interm.nodePair).node1), reinterpret_cast<TIntermTyped*>((yyvsp[-2].interm.nodePair).node2), (yyvsp[0].interm.intermNode), (yylsp[-6]));
(yyval.interm.intermNode) = context->addLoop(ELoopFor, (yyvsp[-3].interm.intermNode), reinterpret_cast<TIntermTyped*>((yyvsp[-2].interm.nodePair).node1), reinterpret_cast<TIntermTyped*>((yyvsp[-2].interm.nodePair).node2), (yyvsp[0].interm.intermNode), (yylsp[-6]));
context->decrLoopNestingLevel();
}
......@@ -4917,7 +4910,7 @@ yyreduce:
case 281:
{
(yyval.interm.intermTypedNode) = (yyvsp[0].interm.intermTypedNode);
(yyval.interm.intermNode) = (yyvsp[0].interm.intermNode);
}
break;
......@@ -4925,7 +4918,7 @@ yyreduce:
case 282:
{
(yyval.interm.intermTypedNode) = 0;
(yyval.interm.intermNode) = nullptr;
}
break;
......@@ -4933,7 +4926,7 @@ yyreduce:
case 283:
{
(yyval.interm.nodePair).node1 = (yyvsp[-1].interm.intermTypedNode);
(yyval.interm.nodePair).node1 = (yyvsp[-1].interm.intermNode);
(yyval.interm.nodePair).node2 = 0;
}
......@@ -4942,7 +4935,7 @@ yyreduce:
case 284:
{
(yyval.interm.nodePair).node1 = (yyvsp[-2].interm.intermTypedNode);
(yyval.interm.nodePair).node1 = (yyvsp[-2].interm.intermNode);
(yyval.interm.nodePair).node2 = (yyvsp[0].interm.intermTypedNode);
}
......
src/tests/gl_tests/GLSLTest.cpp
......@@ -3080,6 +3080,36 @@ TEST_P(WebGLGLSLTest, UninitializedNamelessStructInGlobalScope)
EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
}
// Test that a loop condition that has an initializer declares a variable.
TEST_P(GLSLTest_ES3, ConditionInitializerDeclaresVariable)
{
const std::string &fragmentShader =
"#version 300 es\n"
"precision highp float;\n"
"out vec4 my_FragColor;\n"
"void main()\n"
"{\n"
" float i = 0.0;\n"
" while (bool foo = (i < 1.5))\n"
" {\n"
" if (!foo)\n"
" {\n"
" ++i;\n"
" }\n"
" if (i > 3.5)\n"
" {\n"
" break;\n"
" }\n"
" ++i;\n"
" }\n"
" my_FragColor = vec4(i * 0.5 - 1.0, i * 0.5, 0.0, 1.0);\n"
"}\n";
ANGLE_GL_PROGRAM(program, mSimpleVSSource, fragmentShader);
drawQuad(program.get(), "inputAttribute", 0.5f);
EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
}
// Use this to select which configurations (e.g. which renderer, which GLES major version) these tests should be run against.
ANGLE_INSTANTIATE_TEST(GLSLTest,
ES2_D3D9(),
......
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