Skip to content
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

Implement pass to remove advanced statements #925

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Stagno
Copy link
Contributor

@Stagno Stagno commented Apr 16, 2020

Technical Description

  • Add PassSimplifyStatements to transform compound assignments and increment/decrement ops into simple assignments. And tests.

Resolves / Enhances

first point of #917

@Stagno
Copy link
Contributor Author

Stagno commented Apr 17, 2020

launch jenkins

@Stagno Stagno marked this pull request as ready for review April 17, 2020 07:44
@Stagno Stagno requested a review from cosunae April 17, 2020 08:21
@Stagno
Copy link
Contributor Author

Stagno commented Apr 17, 2020

launch jenkins

@cosunae cosunae mentioned this pull request Apr 22, 2020
6 tasks
@Stagno
Copy link
Contributor Author

Stagno commented Apr 22, 2020

launch jenkins

Comment on lines +66 to +79
// 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;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, need to fix this.

Comment on lines +35 to +44
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);
Copy link
Contributor

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:

if (i == 0 && ++i == 1) { /* ... */ }

will become:

i = i + 1;
if (i == 0 && i == 1) { /* ... */ }

Copy link
Contributor

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)?

Copy link
Contributor

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:

if (i == 0) {
  i = i + 1;
  if (i == 1) { /* ... */ }
}

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):

i = i + 1;
if (i - 1 == 0 && i == 1) { /* ... */ }

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

@Stagno
Copy link
Contributor Author

Stagno commented Aug 12, 2020

launch jenkins

1 similar comment
@Stagno
Copy link
Contributor Author

Stagno commented Aug 13, 2020

launch jenkins

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants