-
Notifications
You must be signed in to change notification settings - Fork 26
Implement pass to remove advanced statements #925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Stagno
wants to merge
7
commits into
MeteoSwiss-APN:master
Choose a base branch
from
Stagno:pass_remove_advanced_statements
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
73a4819
First implementation and tests
Stagno 12c400c
Consider nested increments/decrements + more tests
Stagno 318a5c7
Add to lowering, fix bug, update one ref
Stagno b7a5f4c
Merge branch 'master' into pass_remove_advanced_statements
Stagno bcd330c
Remove usage of CompilerUtils
Stagno 8b35fe3
Merge branch 'master' into pass_remove_advanced_statements
Stagno 5ea58e9
Use new pattern for tests
Stagno File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| //===--------------------------------------------------------------------------------*- C++ -*-===// | ||
| // _ | ||
| // | | | ||
| // __| | __ ___ ___ ___ | ||
| // / _` |/ _` \ \ /\ / / '_ | | ||
| // | (_| | (_| |\ V V /| | | | | ||
| // \__,_|\__,_| \_/\_/ |_| |_| - Compiler Toolchain | ||
| // | ||
| // | ||
| // This file is distributed under the MIT License (MIT). | ||
| // See LICENSE.txt for details. | ||
| // | ||
| //===------------------------------------------------------------------------------------------===// | ||
|
|
||
| #include "PassSimplifyStatements.h" | ||
| #include "dawn/AST/ASTExpr.h" | ||
| #include "dawn/IIR/ASTFwd.h" | ||
| #include "dawn/IIR/AccessComputation.h" | ||
| #include "dawn/IIR/DoMethod.h" | ||
| #include "dawn/IIR/StencilInstantiation.h" | ||
| #include "dawn/Support/Type.h" | ||
| #include <memory> | ||
|
|
||
| namespace dawn { | ||
|
|
||
| namespace { | ||
| class IncrementDecrementReplacer : public ast::ASTVisitorPostOrder { | ||
| std::vector<std::shared_ptr<ast::Stmt>> statements_; | ||
|
|
||
| public: | ||
| std::shared_ptr<iir::Expr> | ||
| postVisitNode(std::shared_ptr<iir::UnaryOperator> const& unaryOp) override { | ||
| auto sourceLoc = unaryOp->getSourceLocation(); | ||
| std::shared_ptr<ast::BinaryOperator> binOp; | ||
| if(unaryOp->getOp() == "++") { | ||
| binOp = std::make_shared<ast::BinaryOperator>( | ||
| unaryOp->getOperand()->clone(), "+", | ||
| std::make_shared<ast::LiteralAccessExpr>("1", BuiltinTypeID::Integer, sourceLoc), | ||
| sourceLoc); | ||
| } else if(unaryOp->getOp() == "--") { | ||
| binOp = std::make_shared<ast::BinaryOperator>( | ||
| unaryOp->getOperand()->clone(), "-", | ||
| std::make_shared<ast::LiteralAccessExpr>("1", BuiltinTypeID::Integer, sourceLoc), | ||
| sourceLoc); | ||
| } else { | ||
| return unaryOp; | ||
| } | ||
| DAWN_ASSERT(unaryOp->getOperand()->getKind() == ast::Expr::Kind::FieldAccessExpr || | ||
| unaryOp->getOperand()->getKind() == ast::Expr::Kind::VarAccessExpr); | ||
| auto newAssignmentExpr = std::make_shared<ast::AssignmentExpr>(unaryOp->getOperand()->clone(), | ||
| binOp, "=", sourceLoc); | ||
|
|
||
| statements_.push_back(iir::makeExprStmt(newAssignmentExpr, sourceLoc)); | ||
|
|
||
| return unaryOp->getOperand(); | ||
| } | ||
| const std::vector<std::shared_ptr<ast::Stmt>>& getReplacements() { return statements_; } | ||
| }; | ||
| } // namespace | ||
|
|
||
| bool PassSimplifyStatements::run( | ||
| const std::shared_ptr<iir::StencilInstantiation>& stencilInstantiation) { | ||
| for(const auto& doMethod : iterateIIROver<iir::DoMethod>(*stencilInstantiation->getIIR())) { | ||
| for(auto stmtIt = doMethod->getAST().getStatements().begin(); | ||
| stmtIt != doMethod->getAST().getStatements().end();) { | ||
| // Compound assignment | ||
| if(const auto& exprStmt = std::dynamic_pointer_cast<iir::ExprStmt>(*stmtIt)) { | ||
| auto sourceLoc = exprStmt->getSourceLocation(); | ||
| if(const auto& assignmentExpr = | ||
| std::dynamic_pointer_cast<iir::AssignmentExpr>(exprStmt->getExpr())) { | ||
| if(assignmentExpr->getOp() != "=") { | ||
| auto binOp = std::make_shared<ast::BinaryOperator>( | ||
| assignmentExpr->getLeft()->clone(), assignmentExpr->getOp().substr(0, 1), | ||
| assignmentExpr->getRight(), sourceLoc); | ||
| auto newAssignmentExpr = std::make_shared<ast::AssignmentExpr>( | ||
| assignmentExpr->getLeft(), binOp, "=", sourceLoc); | ||
| exprStmt->getExpr() = newAssignmentExpr; | ||
| } | ||
| } | ||
|
Comment on lines
+66
to
+79
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems to ignore compoung assignments in nested block statements (e.g., nested ifs)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thx, need to fix this. |
||
| } | ||
|
|
||
| // Increment/decrement ops (can be nested inside expression tree) | ||
| IncrementDecrementReplacer replacer; | ||
| doMethod->getAST().substitute(stmtIt, (*stmtIt)->acceptAndReplace(replacer)); | ||
| stmtIt = doMethod->getAST().insert(stmtIt, replacer.getReplacements().begin(), | ||
| replacer.getReplacements().end()); | ||
| std::advance(stmtIt, replacer.getReplacements().size()); | ||
| // Substitution might have left an useless statement accessing a field/variable. | ||
| if(const auto& exprStmt = std::dynamic_pointer_cast<iir::ExprStmt>(*stmtIt)) { | ||
| if(exprStmt->getExpr()->getKind() == ast::Expr::Kind::FieldAccessExpr || | ||
| exprStmt->getExpr()->getKind() == ast::Expr::Kind::VarAccessExpr) { | ||
| stmtIt = doMethod->getAST().erase(stmtIt); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| ++stmtIt; | ||
| } | ||
| // Recompute the accesses metadata of all statements (new statements and changed statements | ||
| // require this) | ||
| computeAccesses(stencilInstantiation->getMetaData(), doMethod->getAST().getStatements()); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| } // namespace dawn | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| //===--------------------------------------------------------------------------------*- C++ -*-===// | ||
| // _ | ||
| // | | | ||
| // __| | __ ___ ___ ___ | ||
| // / _` |/ _` \ \ /\ / / '_ | | ||
| // | (_| | (_| |\ V V /| | | | | ||
| // \__,_|\__,_| \_/\_/ |_| |_| - Compiler Toolchain | ||
| // | ||
| // | ||
| // This file is distributed under the MIT License (MIT). | ||
| // See LICENSE.txt for details. | ||
| // | ||
| //===------------------------------------------------------------------------------------------===// | ||
|
|
||
| #pragma once | ||
|
|
||
| #include "Pass.h" | ||
|
|
||
| namespace dawn { | ||
|
|
||
| /// @brief PassSimplifyStatements converts "advanced" statements (compound assignments and | ||
| /// increment, decrement ops) into their extended equivalent forms, to have a syntax which is | ||
| /// simpler to anaylise. | ||
| /// @ingroup optimizer | ||
| /// | ||
| /// This pass is necessary to generate legal IIR | ||
| class PassSimplifyStatements : public Pass { | ||
| public: | ||
| PassSimplifyStatements(OptimizerContext& context) : Pass(context, "PassSimplifyStatements") {} | ||
|
|
||
| /// @brief Pass implementation | ||
| bool run(const std::shared_ptr<iir::StencilInstantiation>& stencilInstantiation) override; | ||
| }; | ||
|
|
||
| } // namespace dawn |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
119 changes: 119 additions & 0 deletions
119
dawn/test/unit-test/dawn/Optimizer/TestPassSimplifyStatements.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| //===--------------------------------------------------------------------------------*- C++ -*-===// | ||
| // _ | ||
| // | | | ||
| // __| | __ ___ ___ ___ | ||
| // / _` |/ _` \ \ /\ / / '_ | | ||
| // | (_| | (_| |\ V V /| | | | | ||
| // \__,_|\__,_| \_/\_/ |_| |_| - Compiler Toolchain | ||
| // | ||
| // | ||
| // This file is distributed under the MIT License (MIT). | ||
| // See LICENSE.txt for details. | ||
| // | ||
| //===------------------------------------------------------------------------------------------===// | ||
|
|
||
| #include "dawn/AST/ASTStringifier.h" | ||
| #include "dawn/IIR/ASTFwd.h" | ||
| #include "dawn/IIR/IIR.h" | ||
| #include "dawn/IIR/StencilInstantiation.h" | ||
| #include "dawn/Optimizer/OptimizerContext.h" | ||
| #include "dawn/Optimizer/PassSimplifyStatements.h" | ||
| #include "dawn/Serialization/IIRSerializer.h" | ||
| #include "dawn/Unittest/ASTConstructionAliases.h" | ||
| #include "dawn/Unittest/UnittestUtils.h" | ||
|
|
||
| #include <fstream> | ||
| #include <gtest/gtest.h> | ||
| #include <memory> | ||
|
|
||
| using namespace dawn; | ||
| using namespace astgen; | ||
|
|
||
| namespace { | ||
|
|
||
| std::shared_ptr<iir::StencilInstantiation> initializeInstantiation(const std::string& filename) { | ||
| UIDGenerator::getInstance()->reset(); | ||
| auto instantiation = IIRSerializer::deserialize(filename); | ||
| DiagnosticsEngine diag; | ||
| OptimizerContext context(diag, {}, {{instantiation->getName(), instantiation}}); | ||
|
|
||
| PassSimplifyStatements pass(context); | ||
| pass.run(instantiation); | ||
| EXPECT_TRUE(!diag.hasErrors()); | ||
|
|
||
| return instantiation; | ||
| } | ||
|
|
||
| TEST(TestPassSimplifyStatements, CompoundStatement) { | ||
| // b += a; | ||
| // d -= c; | ||
| auto instantiation = initializeInstantiation("input/test_simplify_statements_compound_statement.iir"); | ||
| auto const& firstStmt = getNthStmt(getFirstDoMethod(instantiation), 0); | ||
| ASSERT_TRUE(firstStmt->equals(expr(assign(field("b"), binop(field("b"), "+", field("a")))).get(), | ||
| /*compareData = */ false)); | ||
| auto const& secondStmt = getNthStmt(getFirstDoMethod(instantiation), 1); | ||
| ASSERT_TRUE(secondStmt->equals(expr(assign(field("d"), binop(field("d"), "-", field("c")))).get(), | ||
| /*compareData = */ false)); | ||
| } | ||
|
|
||
| TEST(TestPassSimplifyStatements, IncrementDecrement) { | ||
| // int b = d; | ||
| // int c = d; | ||
| // --b; | ||
| // ++c; | ||
| // a = c + b; | ||
| auto instantiation = initializeInstantiation("input/test_simplify_statements_increment_decrement.iir"); | ||
| ASSERT_EQ(5, getFirstDoMethod(instantiation).getAST().getStatements().size()); | ||
| auto const& firstStmt = getNthStmt(getFirstDoMethod(instantiation), 2); | ||
| ASSERT_TRUE(firstStmt->equals(expr(assign(var("b"), binop(var("b"), "-", lit(1)))).get(), | ||
| /*compareData = */ false)); | ||
| auto const& secondStmt = getNthStmt(getFirstDoMethod(instantiation), 3); | ||
| ASSERT_TRUE(secondStmt->equals(expr(assign(var("c"), binop(var("c"), "+", lit(1)))).get(), | ||
| /*compareData = */ false)); | ||
| } | ||
|
|
||
| TEST(TestPassSimplifyStatements, IncrementNested) { | ||
| // int b = c; | ||
| // a = (++b) + 1; | ||
| auto instantiation = initializeInstantiation("input/test_simplify_statements_increment_nested.iir"); | ||
| ASSERT_EQ(3, getFirstDoMethod(instantiation).getAST().getStatements().size()); | ||
| auto const& firstStmt = getNthStmt(getFirstDoMethod(instantiation), 1); | ||
| ASSERT_TRUE(firstStmt->equals(expr(assign(var("b"), binop(var("b"), "+", lit(1)))).get(), | ||
| /*compareData = */ false)); | ||
| auto const& secondStmt = getNthStmt(getFirstDoMethod(instantiation), 2); | ||
| ASSERT_TRUE(secondStmt->equals(expr(assign(field("a"), binop(var("b"), "+", lit(1)))).get(), | ||
| /*compareData = */ false)); | ||
| } | ||
|
|
||
| TEST(TestPassSimplifyStatements, MixMultipleNested) { | ||
| // int b = d; | ||
| // int c = d; | ||
| // a += ++b + (1 + --c); | ||
| // a *= ++c * --b; | ||
| auto instantiation = initializeInstantiation("input/test_simplify_statements_mix_multiple_nested.iir"); | ||
| ASSERT_EQ(8, getFirstDoMethod(instantiation).getAST().getStatements().size()); | ||
| auto const& firstStmt = getNthStmt(getFirstDoMethod(instantiation), 2); | ||
| ASSERT_TRUE(firstStmt->equals(expr(assign(var("b"), binop(var("b"), "+", lit(1)))).get(), | ||
| /*compareData = */ false)); | ||
| auto const& secondStmt = getNthStmt(getFirstDoMethod(instantiation), 3); | ||
| ASSERT_TRUE(secondStmt->equals(expr(assign(var("c"), binop(var("c"), "-", lit(1)))).get(), | ||
| /*compareData = */ false)); | ||
| auto const& thirdStmt = getNthStmt(getFirstDoMethod(instantiation), 4); | ||
| ASSERT_TRUE(thirdStmt->equals( | ||
| expr(assign(field("a"), | ||
| binop(field("a"), "+", binop(var("b"), "+", binop(lit(1), "+", var("c")))))) | ||
| .get(), | ||
| /*compareData = */ false)); | ||
| auto const& fourthStmt = getNthStmt(getFirstDoMethod(instantiation), 5); | ||
| ASSERT_TRUE(fourthStmt->equals(expr(assign(var("c"), binop(var("c"), "+", lit(1)))).get(), | ||
| /*compareData = */ false)); | ||
| auto const& fifthStmt = getNthStmt(getFirstDoMethod(instantiation), 6); | ||
| ASSERT_TRUE(fifthStmt->equals(expr(assign(var("b"), binop(var("b"), "-", lit(1)))).get(), | ||
| /*compareData = */ false)); | ||
| auto const& sixthStmt = getNthStmt(getFirstDoMethod(instantiation), 7); | ||
| ASSERT_TRUE(sixthStmt->equals( | ||
| expr(assign(field("a"), binop(field("a"), "*", binop(var("c"), "*", var("b"))))).get(), | ||
| /*compareData = */ false)); | ||
| } | ||
|
|
||
| } // namespace |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't work in this case:
will become:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting one! Can we resolve this (without introducing a temporary variable)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A valid transformation output would be:
But I don't think that such an output would be suitable for a general transformation algorithm.
Note: this isn't equivalent if SIR also has C++'s short-circuit semantics (it would also introduce more ops):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is not an easy problem to solve, let's leave this open for the next syntax workshop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. But do we then merge a pass that only works for a subset of SIR and could change semantics if SIR outside that subset is given?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenWeber42 I wouldn't merge it for now. Let's briefly present the problem at the review meeting. A quick solution could be saying that increment and decrement ops can't be nested in expressions (i.e. only allowed in statements like i++; i--;) from the beginning (frontends/SIR). If this solution is not widely accepted, then this problem will need to be discussed more in detail, because it seems that the only way to solve it is to do the heavy transformation that you showed.