Use side-effects of user defined functions in evm code transform.

[chriseth]

trafficstars

I tried but failed. Assigning to @ekpyron ~Depends on https://github.com/ethereum/solidity/pull/12759 and https://github.com/ethereum/solidity/pull/12795~

[ekpyron]

The remaining failing test is due to the new code transform allowing arbitrary inputs so far, but it's also perfectly fine to require disambiguated, hoisted, etc. code, so the failure is fine.

I just quickly ran the semantics tests on a merge of this with https://github.com/ethereum/solidity/pull/11974 and they all pass. And it does have effects: on affected tests I either saw a slight decrease by a few gas or a significant decrease by up to ~1% of gas. Funnily, I also saw code size increase occasionally :-). Which pretty much goes along with my suspicion that good parts of the code size increase in the new code transform is due to needlessly cleaning up the stack before a revert(0,0) or the like (i.e. an array of senseless pops before). So once we make the new code transform clever about that, the code size concern might just vanish.

There is a paranoid assertion in OptimizedCodeTransform::operator()(CFG::BasicBlock const& _block) (L500+) that checks on blocks with CFG::BasicBlock::Terminated exits that the last operation was actually a terminating builtin... if we want to stay that paranoid, we'd need to pass in the side effects there, too, but it should also be fine to just remove that assertion.

[chriseth]

Ok this is weird - now there are gas changes...

[chriseth]

Ok, now I understand what @ekpyron was talking about in the last days.

let a, b, c, d, e, f, g = x();
if a { t() }
...

In the above, we have a rather deep stack at the point where we call t. If the analysis shows that t is always terminating, the block is marked as "terminating". The problem is now that the stack is cleaned up at the end of each block marked as "terminating". This results in an least 7 pop opcodes being introduced at the end of the if branch. Before this change, the if branch would just join again with the main branch and thus would have no cleanup at all.

So I actually think that the "allow garbage at the end of terminating branches" feature is really important to implement. The deposit contract contains the following code (highlights are from a diff):

[chriseth]

Another optimization here could be that we should not push a return tag for a call to a terminating function.

[ekpyron]

This has more significant gains compared to https://github.com/ethereum/solidity/pull/12759

bleeps
   ir-optimize-evm+yul
     bytecode_size :  -3.09 %
     method_gas :  -0.01 %
elementfi
   ir-optimize-evm+yul
     bytecode_size :  -5.65 %
     deployment_gas :  -5.17 %
     method_gas :  -2.02 %
ens
   ir-optimize-evm+yul
     bytecode_size :  -4.66 %
     deployment_gas :  -4.38 %
     method_gas :  -0.05 %
gnosis
   ir-optimize-evm+yul
     bytecode_size :  -3.36 %
     deployment_gas :  -3.34 %
     method_gas :  -0.02 %
pool-together
   ir-optimize-evm+yul
     bytecode_size :  -4.16 %
     deployment_gas :  -4.08 %
     method_gas :  -0.02 %
prb-math
   ir-optimize-evm+yul
     bytecode_size :  -1.61 %
     deployment_gas :  -1.58 %
trident
   ir-optimize-evm+yul
     bytecode_size :  -4.69 %
     deployment_gas :  -4.40 %
     method_gas :  -4.43 %
yield_liquidator
   ir-optimize-evm+yul
     bytecode_size :  -3.06 %
     deployment_gas :  -3.07 %
     method_gas :  -0.01 %
zeppelin
   ir-optimize-evm+yul
     bytecode_size :  -3.74 %
     deployment_gas :  -3.21 %
     method_gas :  -0.03 %

In total relative to develop:

bleeps
   ir-optimize-evm+yul
     bytecode_size :  -4.02 %
     method_gas :  -0.01 %
elementfi
   ir-optimize-evm+yul
     bytecode_size :  -7.02 %
     deployment_gas :  -6.25 %
     method_gas :  -2.54 %
ens
   ir-optimize-evm+yul
     bytecode_size :  -6.44 %
     deployment_gas :  -5.93 %
     method_gas :  -0.03 %
gnosis
   ir-optimize-evm+yul
     bytecode_size :  -5.17 %
     deployment_gas :  -4.89 %
     method_gas :  -0.02 %
pool-together
   ir-optimize-evm+yul
     bytecode_size :  -5.79 %
     deployment_gas :  -5.49 %
     method_gas :  -0.02 %
prb-math
   ir-optimize-evm+yul
     bytecode_size :  -2.22 %
     deployment_gas :  -2.16 %
trident
   ir-optimize-evm+yul
     bytecode_size :  -6.01 %
     deployment_gas :  -5.55 %
     method_gas :  -5.00 %
yield_liquidator
   ir-optimize-evm+yul
     bytecode_size :  -4.17 %
     deployment_gas :  -3.97 %
     method_gas :  -0.01 %
zeppelin
   ir-optimize-evm+yul
     bytecode_size :  -5.59 %
     deployment_gas :  -4.80 %
     method_gas :  -0.04 %

[leonardoalt]

Dependency was merged, so not so much left to do here

[ekpyron]

This is the output of the benchmark differ (https://github.com/ethereum/solidity/pull/12804) for the IR-OPTIMIZE-EVM+YUL run (the rest remains unaffected):

ir-optimize-evm+yul

project	bytecode_size	deployment_gas	method_gas
bleeps	-3.07% ✅	0%	-0.01% ✅
colony	0%
elementfi	-5.61% ✅
ens	-4.62% ✅	-4.36% ✅	-0.08% ✅
euler
gnosis	-3.32% ✅	-3.23% ✅	-0.02% ✅
perpetual-pools
pool-together	-4.14% ✅	-4.07% ✅	-0.02% ✅
prb-math	-1.61% ✅	-1.58% ✅	0%
trident	-4.74% ✅	-4.49% ✅	-4.45% ✅
uniswap
yield_liquidator	-3.04% ✅	-3.03% ✅	-0.01% ✅
zeppelin	-3.65% ✅	-3.13% ✅	+0.02% ❌

I would have expected more of a runtime difference - but it may be that this largely kicks in on reverting paths and those don't occur as much in the method gas benchmarks, but not entirely sure... and the code size difference alone is worthwhile anyways.

[cameel]

Rebased on develop, fixed conflicts.

[chriseth]

Why did you close this?

[cameel]

I mistakenly deleted the wrong branch and github closed it. Reopened.

[ekpyron]

Rebased again - my guess is that this has less of an effect after https://github.com/ethereum/solidity/pull/12731 now, but I'd say we should still do it.

[cameel]

If you want to see the benchmark diff for this PR you should rebase on develop again. I just merged #13097 to fix the c_ext_benchmark job. Without the fix there's no baseline benchmark available on develop and diffing will fail.

[ekpyron]

Still seems to have some positive effects, even though mainly on bytecode size apparently:

project	bytecode_size	deployment_gas	method_gas
bleeps	-1.6%	0%	-0.01%
brink	-1.22%
colony	0%
elementfi	-3.87%
ens	0%	0%	0%
euler	-2.7%	-2.64%	-0.01%
gnosis	-2.55%	-2.13%	-0.02%
perpetual-pools	-1.55%	-1.61%	-0.05%
pool-together	-1.45%	-1.55%	-0.01%
prb-math	0%	+0%	0%
trident	-0.29%	-0.56%	-0%
uniswap	0%	-0%	+0%
yield_liquidator	-0.47%	-0.52%	-0%
zeppelin	-0.07%	-0.08%	-0%

[ekpyron]

[rebased again]

[leonardoalt]

@ekpyron @cameel how about this PR?

[ekpyron]

@ekpyron @cameel how about this PR?

Sigh, I thought this was also already merged ages ago :-). I'll rebase it and we can hope for another review :-).

[Marenz]

I rebased this and updated the gas costs

[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]

Unstaled and rebased this once more - this is mainly in dire need of reviews.

If anyone is up for this, ping me and I can walk you trough the PR to help in reviewing.

[chriseth]

Code looks good!