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 common subexpression elimination #550

Merged

Conversation

LucasSte
Copy link
Contributor

@LucasSte LucasSte commented Nov 4, 2021

This PR implements common subexpression elimination for the Solang compiler. It runs an available expression analysis during the first pass. Then, during a second pass we exchange common expressions by a temporaries.

This PR is the last milestone for the Linux Mentorship for the Hyperledger foundation. For more information about the project, please check the wiki.

@LucasSte LucasSte force-pushed the common-subexpression-elimination branch from 3a61cb2 to c74fec1 Compare November 4, 2021 14:20
examples/test.sol Outdated Show resolved Hide resolved
@seanyoung
Copy link
Contributor

The codegen test fails randomly, that's why the mac/windows tests have failed.

@LucasSte LucasSte force-pushed the common-subexpression-elimination branch from 87ffbd6 to a4cdd1b Compare November 6, 2021 23:08
@seanyoung
Copy link
Contributor

Please can you merge your commits into one commit, or more than one if it makes sense. It would be nice if commits are a set of logical changes.

Also please ensure your commits have the correct Signed-off-by: line, this is what the DCO failure is about.

@LucasSte LucasSte force-pushed the common-subexpression-elimination branch 2 times, most recently from 143c08f to 53074dc Compare November 10, 2021 12:53
@LucasSte LucasSte marked this pull request as ready for review November 10, 2021 21:31
@LucasSte LucasSte requested a review from seanyoung as a code owner November 10, 2021 21:31
Copy link
Contributor

@seanyoung seanyoung left a comment

Choose a reason for hiding this comment

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

Please add some words to docs/optimizer.rst. The documentation should at least explain that there is a CSE pass, and what it does (not necessarily how it works)

@@ -1254,6 +1257,10 @@ pub fn optimize_and_check_cfg(
if opt.dead_storage {
dead_storage::dead_storage(cfg, ns);
}

if opt.common_subexpression_elimination && func_no.is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for func_no.some()?

A default constructor has no function number, so it is None in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A default constructor has no sub expression to be optimized.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, although that is worth a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@LucasSte LucasSte force-pushed the common-subexpression-elimination branch from 4990368 to 1ff5dc8 Compare November 12, 2021 00:10
@LucasSte LucasSte requested a review from seanyoung November 12, 2021 00:13
@seanyoung seanyoung merged commit 8f19e1b into hyperledger-solang:main Nov 12, 2021
@LucasSte LucasSte deleted the common-subexpression-elimination branch November 14, 2021 12:51
seanyoung added a commit to seanyoung/solang that referenced this pull request Mar 1, 2022
Added
- On Solana, the accounts that were passed into the transactions are listed in
  the `tx.accounts` builtin. There is also a builtin struct `AccountInfo`
- A new common subexpression elimination pass was added, thanks to
  [LucasSte](hyperledger-solang#550)
- A graphviz dot file can be generated from the ast, using `--emit ast-dot`
- Many improvements to the solidity parser, and the parser has been spun out
  in it's own create `solang-parser`.

Changed
- Solang now uses LLVM 13.0, based on the [Solana LLVM tree](https://github.com/solana-labs/llvm-project/)
- The ast datastructure has been simplified.
- Many bugfixes across the entire tree.

Signed-off-by: Sean Young <[email protected]>
seanyoung added a commit to seanyoung/solang that referenced this pull request Mar 1, 2022
Added
- On Solana, the accounts that were passed into the transactions are listed in
  the `tx.accounts` builtin. There is also a builtin struct `AccountInfo`
- A new common subexpression elimination pass was added, thanks to
  [LucasSte](hyperledger-solang#550)
- A graphviz dot file can be generated from the ast, using `--emit ast-dot`
- Many improvements to the solidity parser, and the parser has been spun out
  in it's own create `solang-parser`.

Changed
- Solang now uses LLVM 13.0, based on the [Solana LLVM tree](https://github.com/solana-labs/llvm-project/)
- The ast datastructure has been simplified.
- Many bugfixes across the entire tree.

Signed-off-by: Sean Young <[email protected]>
seanyoung added a commit that referenced this pull request Mar 1, 2022
Added
- On Solana, the accounts that were passed into the transactions are listed in
  the `tx.accounts` builtin. There is also a builtin struct `AccountInfo`
- A new common subexpression elimination pass was added, thanks to
  [LucasSte](#550)
- A graphviz dot file can be generated from the ast, using `--emit ast-dot`
- Many improvements to the solidity parser, and the parser has been spun out
  in it's own create `solang-parser`.

Changed
- Solang now uses LLVM 13.0, based on the [Solana LLVM tree](https://github.com/solana-labs/llvm-project/)
- The ast datastructure has been simplified.
- Many bugfixes across the entire tree.

Signed-off-by: Sean Young <[email protected]>
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.

2 participants