Change the constant optimizer to make use of `PUSH0`

[axic]

Part of #14073.

This change only affects large copies using codecopy (which are supposedly rare).

(It will be fun changing this code for EOF, i.e. use DATACOPY or replace it entirely with a single DATALOADN.)

[github-actions[bot]]

This pull request is stale because it has been open for 14 days with no activity. It will be closed in 7 days unless the stale label is removed.

[github-actions[bot]]

This pull request is stale because it has been open for 14 days with no activity. It will be closed in 7 days unless the stale label is removed.

[ekpyron]

For some reason it has a merge conflict in the peephole optimizer now, so that at least needs to be resolved. But maybe that's all of it now, so ping me once that's done, then I'll have another look!

[matheusaaguiar]

Here's the gas benchmark diff.

Benchmark diff

ir-no-optimize

project	bytecode_size	deployment_gas	method_gas
brink	+0%
colony	+0.04% ❌
elementfi	+0%
ens	+0%
perpetual-pools	+0%	+0%	-0.01% ✅
uniswap	0%	+0%	-0%
yield_liquidator	+0%	+0%	0%
zeppelin

ir-optimize-evm+yul

project	bytecode_size	deployment_gas	method_gas
brink	0%
colony	+0.07% ❌
elementfi	-0.01% ✅
ens	-0.04% ✅	0%	-0%
perpetual-pools	+0%	+0%	-0%
uniswap	-0%	+0%	-0%
yield_liquidator	+0.27% ❌	+0.27% ❌	-0.01% ✅
zeppelin	-0%	+0.01% ❌	-0.06% ✅

ir-optimize-evm-only

project	bytecode_size	deployment_gas	method_gas
brink	0%
colony	+0.06% ❌
elementfi	-0.01% ✅
ens	-0.01% ✅	-0%	0%
perpetual-pools	+0%	+0%	+0.02% ❌
uniswap	-0.01% ✅	+0%	+0%
yield_liquidator	0%	+0%	0%
zeppelin	-0.01% ✅

legacy-no-optimize

project	bytecode_size	deployment_gas	method_gas
brink	+0.03% ❌
colony	+0.04% ❌
elementfi	+0.1% ❌
ens	+0.04% ❌
perpetual-pools	+0.03% ❌	+0.02% ❌	+0%
uniswap	+0.02% ❌	+0.01% ❌	+0%
yield_liquidator	+0.04% ❌	+0.04% ❌	-0.01% ✅
zeppelin	+0.06% ❌	+0.05% ❌	+0.05% ❌

legacy-optimize-evm+yul

project	bytecode_size	deployment_gas	method_gas
brink	+0.21% ❌
colony	+0.07% ❌
elementfi	+0.06% ❌
ens	+0.11% ❌	+0.04% ❌	-0%
perpetual-pools	+0.04% ❌	+0.03% ❌	-0.01% ✅
uniswap	+0.06% ❌	+0.07% ❌	+0.01% ❌
yield_liquidator	+0.04% ❌	+0.04% ❌	-0%
zeppelin	+0.01% ❌	+0.02% ❌	+0.24% ❌

legacy-optimize-evm-only

project	bytecode_size	deployment_gas	method_gas
brink	+0.21% ❌
colony	+0.06% ❌
elementfi	+0.06% ❌
ens	+0.11% ❌	+0.04% ❌	-0%
perpetual-pools	+0.03% ❌	+0.03% ❌	-0.03% ✅
uniswap	+0.06% ❌	+0.07% ❌	+0.01% ❌
yield_liquidator	+0.04% ❌	+0.04% ❌	+0%
zeppelin	+0.01% ❌	+0.02% ❌	+0.02% ❌

!V = version mismatch !B = no value in the "before" version !A = no value in the "after" version !T = one or both values were not numeric and could not be compared -0 = very small negative value rounded to zero +0 = very small positive value rounded to zero

[ekpyron]

What we could still do to offset the regressions would be to add another peephole optimization - if for any pattern like

[AssemblyItem A] [AssemblyItem B] [AssemblyItem C] [TAG] [AssemblyItem X] [AssemblyItem Y] [AssemblyItem Z]

Where A == X && B == Y && C == Z and C is control flow breaking (it terminates or reverts or jumps unconditionally), we can replace this with

[TAG] [AssemblyItem A] [AssemblyItem B] [AssemblyItem C]

But I wonder whether there's a nicer way to do this than that... this is actually closer to block deduplication than to peephole optimization, but we cannot really use that mechanism, since one of the blocks has to remain (since it's fallen into from previous code) - and thereby also doesn't have a jumpdest/tag to mark it...

Due to that, the quickest solution would probably be to add such peephole optimization rules - it should be sufficient to add them for sequences of one, two or three assembly items, that should cover all these cases that are artifacts of partial inlining...

[github-actions[bot]]

This pull request is stale because it has been open for 14 days with no activity. It will be closed in 7 days unless the stale label is removed.

[github-actions[bot]]

This pull request is stale because it has been open for 14 days with no activity. It will be closed in 7 days unless the stale label is removed.

[github-actions[bot]]

This pull request is stale because it has been open for 14 days with no activity. It will be closed in 7 days unless the stale label is removed.

[github-actions[bot]]

This pull request is stale because it has been open for 14 days with no activity. It will be closed in 7 days unless the stale label is removed.

[github-actions[bot]]

This pull request is stale because it has been open for 14 days with no activity. It will be closed in 7 days unless the stale label is removed.

[matheusaaguiar]

I investigated the last 6 problematic cases of the hardhat tests and all cases are related to the introduction of the DeduplicateNextTagSizeX methods in https://github.com/ethereum/solidity/pull/14117/commits/9b1792f4000cfa950391a6e367cf5b327436a3c7. They open up some possible optimizations that result in a different enough generated code that causes the failings in the hardhat stack trace tests since they rely on certain patterns being produced. The first one that appears on the logs, for example, checks function test with b=false in the following solidity code:

pragma solidity ^0.8.0;

contract C {

  modifier m2(bool b)  {
    require(b);
    _;
  }

  function test(bool b) m1(b) m2(b) public {
    revert();
  }

  modifier m1(bool b)  {
    _;
  }

}

The stack trace expectation shows it expected the revert error raised from the require(b); at line 6:

{
  "transactions": [
    {
      "file": "c.sol",
      "contract": "C"
    },
    {
      "to": 0,
      "params": [false],
      "function": "test",
      "stackTrace": [
        {
          "type": "CALLSTACK_ENTRY",
          "sourceReference": {
            "contract": "C",
            "file": "c.sol",
            "function": "test",
            "line": 10
          }
        },
        {
          "type": "REVERT_ERROR",
          "sourceReference": {
            "contract": "C",
            "file": "c.sol",
            "function": "m2",
            "line": 6
          }
        }
      ]
    }
  ]
}

The code generated by the compiler before this PR contains the part related to that:

    tag_7:
        /* "c.sol":119:120  b */
      dup1
        /* "c.sol":125:126  b */
      dup2
        /* "c.sol":76:77  b */
      dup1
        /* "c.sol":68:78  require(b) */
      tag_10
      jumpi
      0x00
      dup1
      revert
    tag_10:
        /* "c.sol":141:149  revert() */
      0x00
      dup1
      revert

But the code generated with the introduction of this PR is optimized further and removes the asm related to the require, according to these steps: (Already assuming all revert dup1 0x00 transformed to revert 0x00 0x00 because it is cheaper than dup1)

1. Remove revert 0x00 0x00 before tag_10 which does exactly the same (PeepholeOptimiser::DeduplicateNextTagSize3)
2. Remove jumpi tag_10 because now it is right before tag_10. Also note that as a consequence dup1 is also removed since it is the condition of the jumpi (PeepholeOptimiser::JumpToNext)
3. Remove tag_10 (only the tag itself, not the instructions following it) because its only reference was removed in step 2 (JumpdestRemover::optimise)
4. The remaining dup2 dup1 before revert 0x00 0x00 are removed (PeepholeOptimiser::OpReturnRevert)

The final result contains only the code corresponding to the revert in line 11:

    tag_7:
        /* "c.sol":141:149  revert() */
      revert(0x00, 0x00)

The require was completely removed from the asm, which makes sense from an optimization point of view, and that causes a mismatch with the expected stack trace. The other failing tests are similar cases where a specific snippet of code is optimized out while that was not the case before.

[matheusaaguiar]

Gas cost benchmarks

ir-no-optimize

project	bytecode_size	deployment_gas	method_gas
brink	+0%
colony	+0.04% ❌
elementfi	+0%
ens	+0%
euler
gnosis
gp2	-0.02% ✅
pool-together	0%
uniswap	0%
yield_liquidator	+0%	+0%	0%
zeppelin

ir-optimize-evm+yul

project	bytecode_size	deployment_gas	method_gas
brink	0%
colony	+0.02% ❌
elementfi	-0%
ens	-0%	0%	-0%
euler	-0%
gnosis
gp2	-0.03% ✅
pool-together	+0%
uniswap	-0%
yield_liquidator	+0.27% ❌	+0.3% ❌	-0.01% ✅
zeppelin	-0.01% ✅	-0.01% ✅	+0.13% ❌

ir-optimize-evm-only

project	bytecode_size	deployment_gas	method_gas
brink	+0.01% ❌
colony	+0.01% ❌
elementfi	-0.01% ✅
ens	-0%	+0.01% ❌	0%
euler
gnosis
gp2	+0.01% ❌
pool-together	+0.01% ❌
uniswap	-0.01% ✅
yield_liquidator	+0%	+0%	0%
zeppelin	-0.01% ✅

legacy-no-optimize

project	bytecode_size	deployment_gas	method_gas
brink	+0.03% ❌
colony	+0.04% ❌
elementfi	+0.1% ❌
ens	+0.04% ❌
euler	+0.04% ❌
gnosis	+0.05% ❌
gp2	-0%
pool-together	+0.06% ❌
uniswap	+0.02% ❌
yield_liquidator	+0.04% ❌	+0.04% ❌	-0.01% ✅
zeppelin	+0.06% ❌	+3.36% ❌	+0.01% ❌

legacy-optimize-evm+yul

project	bytecode_size	deployment_gas	method_gas
brink	0%
colony	+0.02% ❌
elementfi	+0.01% ❌
ens	+0.02% ❌	+0%	-0%
euler	+0.02% ❌
gnosis	0%
gp2	-0.06% ✅
pool-together	+0.01% ❌
uniswap	-0%
yield_liquidator	0%	+0%	-0%
zeppelin	-0.02% ✅	-0%	+0.07% ❌

legacy-optimize-evm-only

project	bytecode_size	deployment_gas	method_gas
brink	0%
colony	+0.01% ❌
elementfi	+0.01% ❌
ens	+0.02% ❌	-0%	-0%
euler	+0.04% ❌
gnosis	0%
gp2	-0.05% ✅
pool-together	-0%
uniswap	-0%
yield_liquidator	0%	-0%	-0%
zeppelin	-0.01% ✅	-0%	-0%

!V = version mismatch !B = no value in the "before" version !A = no value in the "after" version !T = one or both values were not numeric and could not be compared -0 = very small negative value rounded to zero +0 = very small positive value rounded to zero

[github-actions[bot]]

This pull request is stale because it has been open for 14 days with no activity. It will be closed in 7 days unless the stale label is removed.