diff --git a/Changelog.md b/Changelog.md index c14af7738..3d30f5d20 100644 --- a/Changelog.md +++ b/Changelog.md @@ -13,6 +13,7 @@ Compiler Features: Bugfixes: * AST: Fix wrong initial ID for Yul nodes in the AST. * Code Generator: Fix output from via-IR code generator being dependent on which files were discovered by import callback. In some cases, a different AST ID assignment would alter the order of functions in internal dispatch, resulting in superficially different but semantically equivalent bytecode. + * Common Subexpression Eliminator: Fix replacement decisions being affected by Yul variable names generated by the compiler, resulting in different (but equivalent) bytecode in some situations. * NatSpec: Fix internal error when requesting userdoc or devdoc for a contract that emits an event defined in a foreign contract or interface. * SMTChecker: Fix encoding error that causes loops to unroll after completion. * SMTChecker: Fix inconsistency on constant condition checks when ``while`` or ``for`` loops are unrolled before the condition check. diff --git a/libyul/optimiser/DataFlowAnalyzer.cpp b/libyul/optimiser/DataFlowAnalyzer.cpp index 958e70a83..5992c9b98 100644 --- a/libyul/optimiser/DataFlowAnalyzer.cpp +++ b/libyul/optimiser/DataFlowAnalyzer.cpp @@ -351,13 +351,14 @@ void DataFlowAnalyzer::clearValues(std::set _variables) }); // Also clear variables that reference variables to be cleared. + std::set referencingVariables; for (auto const& variableToClear: _variables) for (auto const& [ref, names]: m_state.references) if (names.count(variableToClear)) - _variables.emplace(ref); + referencingVariables.emplace(ref); // Clear the value and update the reference relation. - for (auto const& name: _variables) + for (auto const& name: _variables + referencingVariables) { m_state.value.erase(name); m_state.references.erase(name); diff --git a/test/cmdlineTests/~name_dependent_cse_bug/cse_bug.yul b/test/cmdlineTests/~name_dependent_cse_bug/cse_bug.yul new file mode 100644 index 000000000..2ed2f27ff --- /dev/null +++ b/test/cmdlineTests/~name_dependent_cse_bug/cse_bug.yul @@ -0,0 +1,30 @@ +object "C" { + code {} + + object "C_deployed" { + code { + main(0, 0) + + function main(a, b) { + for {} 1 {} + { + if iszero(a) { break } + + let mid := avg(a, a) + switch a + case 0 { + a := mid + } + default { + sstore(0, mid) + } + } + } + + function avg(x, y) -> var { + let __placeholder__ := add(x, y) + var := add(__placeholder__, __placeholder__) + } + } + } +} diff --git a/test/cmdlineTests/~name_dependent_cse_bug/test.sh b/test/cmdlineTests/~name_dependent_cse_bug/test.sh new file mode 100755 index 000000000..96c3ac23e --- /dev/null +++ b/test/cmdlineTests/~name_dependent_cse_bug/test.sh @@ -0,0 +1,28 @@ +#!/usr/bin/env bash +set -eo pipefail + +# This is a regression test against https://github.com/ethereum/solidity/issues/14494 +# Due to the bug, a decision about which variable to use to replace a subexpression in CSE would +# depend on sorting order of variable names. A variable not being used as a replacement could lead +# to it being unused in general and removed by Unused Pruner. This would show up as a difference +# in the bytecode. + +# shellcheck source=scripts/common.sh +source "${REPO_ROOT}/scripts/common.sh" +# shellcheck source=scripts/common_cmdline.sh +source "${REPO_ROOT}/scripts/common_cmdline.sh" + +SCRIPT_DIR=$(cd "$(dirname "$0")" && pwd) + +function assemble_with_variable_name { + local input_file="$1" + local variable_name="$2" + + sed -e "s|__placeholder__|${variable_name}|g" "$input_file" | msg_on_error --no-stderr \ + "$SOLC" --strict-assembly - --optimize --debug-info none | + stripCLIDecorations +} + +diff_values \ + "$(assemble_with_variable_name "${SCRIPT_DIR}/cse_bug.yul" _1)" \ + "$(assemble_with_variable_name "${SCRIPT_DIR}/cse_bug.yul" _2)" diff --git a/test/libsolidity/semanticTests/externalContracts/prbmath_unsigned.sol b/test/libsolidity/semanticTests/externalContracts/prbmath_unsigned.sol index 77a1e2ca6..9efc71108 100644 --- a/test/libsolidity/semanticTests/externalContracts/prbmath_unsigned.sol +++ b/test/libsolidity/semanticTests/externalContracts/prbmath_unsigned.sol @@ -48,7 +48,7 @@ contract test { } // ---- // constructor() -// gas irOptimized: 1722610 +// gas irOptimized: 1722166 // gas legacy: 2210160 // gas legacyOptimized: 1734152 // div(uint256,uint256): 3141592653589793238, 88714123 -> 35412542528203691288251815328 diff --git a/test/libsolidity/semanticTests/externalContracts/ramanujan_pi.sol b/test/libsolidity/semanticTests/externalContracts/ramanujan_pi.sol index a1f1d6395..45fb4486d 100644 --- a/test/libsolidity/semanticTests/externalContracts/ramanujan_pi.sol +++ b/test/libsolidity/semanticTests/externalContracts/ramanujan_pi.sol @@ -33,7 +33,7 @@ contract test { } // ---- // constructor() -// gas irOptimized: 407075 +// gas irOptimized: 406595 // gas legacy: 631753 // gas legacyOptimized: 459425 // prb_pi() -> 3141592656369545286 diff --git a/test/libsolidity/semanticTests/externalContracts/strings.sol b/test/libsolidity/semanticTests/externalContracts/strings.sol index 226bf5947..a90b3e83b 100644 --- a/test/libsolidity/semanticTests/externalContracts/strings.sol +++ b/test/libsolidity/semanticTests/externalContracts/strings.sol @@ -49,7 +49,7 @@ contract test { } // ---- // constructor() -// gas irOptimized: 634328 +// gas irOptimized: 633680 // gas legacy: 1065857 // gas legacyOptimized: 725423 // toSlice(string): 0x20, 11, "hello world" -> 11, 0xa0 @@ -69,6 +69,6 @@ contract test { // gas legacy: 31621 // gas legacyOptimized: 27914 // benchmark(string,bytes32): 0x40, 0x0842021, 8, "solidity" -> 0x2020 -// gas irOptimized: 1981677 +// gas irOptimized: 1981664 // gas legacy: 4235651 // gas legacyOptimized: 2319622 diff --git a/test/libyul/yulOptimizerTests/fullSuite/name_dependent_cse_bug_part_1.yul b/test/libyul/yulOptimizerTests/fullSuite/name_dependent_cse_bug_part_1.yul new file mode 100644 index 000000000..f0d4b49b1 --- /dev/null +++ b/test/libyul/yulOptimizerTests/fullSuite/name_dependent_cse_bug_part_1.yul @@ -0,0 +1,45 @@ +{ + main(0, 0) + + function main(a, b) { + for {} 1 {} + { + if iszero(a) { break } + + let mid := avg(a, a) + switch a + case 0 { + a := mid + } + default { + sstore(0, mid) + } + } + } + + function avg(x, y) -> var { + // NOTE: Variable names should not affect CSE. + // This should not be optimized differently than name_dependent_cse_bug_part_2.yul. + // `let mid := var` should be present in both or missing in both. + let _1 := add(x, y) + var := add(_1, _1) + } +} +// ==== +// EVMVersion: >=shanghai +// ---- +// step: fullSuite +// +// { +// { +// let a := 0 +// for { } a { } +// { +// let _1 := add(a, a) +// let var := add(_1, _1) +// switch a +// case 0 { a := var } +// default { sstore(0, var) } +// } +// } +// } diff --git a/test/libyul/yulOptimizerTests/fullSuite/name_dependent_cse_bug_part_1_pre_shanghai.yul b/test/libyul/yulOptimizerTests/fullSuite/name_dependent_cse_bug_part_1_pre_shanghai.yul new file mode 100644 index 000000000..d041cff10 --- /dev/null +++ b/test/libyul/yulOptimizerTests/fullSuite/name_dependent_cse_bug_part_1_pre_shanghai.yul @@ -0,0 +1,46 @@ +{ + main(0, 0) + + function main(a, b) { + for {} 1 {} + { + if iszero(a) { break } + + let mid := avg(a, a) + switch a + case 0 { + a := mid + } + default { + sstore(0, mid) + } + } + } + + function avg(x, y) -> var { + // NOTE: Variable names should not affect CSE. + // This should not be optimized differently than name_dependent_cse_bug_part_2_pre_shanghai.yul. + // `let mid := var` should be present in both or missing in both. + let _1 := add(x, y) + var := add(_1, _1) + } +} +// ==== +// EVMVersion: var { + // NOTE: Variable names should not affect CSE. + // This should not be optimized differently than name_dependent_cse_bug_part_1.yul. + // `let mid := var` should be present in both or missing in both. + let _2 := add(x, y) + var := add(_2, _2) + } +} +// ==== +// EVMVersion: >=shanghai +// ---- +// step: fullSuite +// +// { +// { +// let a := 0 +// for { } a { } +// { +// let _1 := add(a, a) +// let var := add(_1, _1) +// switch a +// case 0 { a := var } +// default { sstore(0, var) } +// } +// } +// } diff --git a/test/libyul/yulOptimizerTests/fullSuite/name_dependent_cse_bug_part_2_pre_shanghai.yul b/test/libyul/yulOptimizerTests/fullSuite/name_dependent_cse_bug_part_2_pre_shanghai.yul new file mode 100644 index 000000000..06965e3ca --- /dev/null +++ b/test/libyul/yulOptimizerTests/fullSuite/name_dependent_cse_bug_part_2_pre_shanghai.yul @@ -0,0 +1,46 @@ +{ + main(0, 0) + + function main(a, b) { + for {} 1 {} + { + if iszero(a) { break } + + let mid := avg(a, a) + switch a + case 0 { + a := mid + } + default { + sstore(0, mid) + } + } + } + + function avg(x, y) -> var { + // NOTE: Variable names should not affect CSE. + // This should not be optimized differently than name_dependent_cse_bug_part_1_pre_shanghai.yul. + // `let mid := var` should be present in both or missing in both. + let _2 := add(x, y) + var := add(_2, _2) + } +} +// ==== +// EVMVersion: