-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Remove inlining constraint for large functions when targetting the new code transform. #12731
Conversation
Changelog.md
Outdated
@@ -5,6 +5,7 @@ Language Features: | |||
|
|||
Compiler Features: | |||
* JSON-AST: Added selector field for errors and events. | |||
* Yul Optimizer: Improve inlining heauristics for via IR code generation and pure Yul compilation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Yul Optimizer: Improve inlining heauristics for via IR code generation and pure Yul compilation. | |
* Yul Optimizer: Improve inlining heuristic for via IR code generation and pure Yul compilation. |
mstore(add(add(headStart, length), 96), 0) | ||
} | ||
tail := add(add(headStart, and(add(length, 31), not(31))), 96) | ||
} | ||
function panic_error_0x32() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it not inline those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently it has a code size of at least 6 and thereby generally doesn't qualify...
@@ -57,6 +57,24 @@ | |||
// let a_56 := b_54 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this test still meaningful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, but we may want to run it in a setup that'll go through the old code transform...
On our tests we get this. But more curious about the external test stats. Even more so since they distinguish code size from gas cost better. Click for a table of gas differences
|
8b11252
to
8fef645
Compare
caf191b
to
46d8611
Compare
8fef645
to
7f1f87a
Compare
test/externalTests/elementfi.sh
Outdated
@@ -89,6 +89,8 @@ function elementfi_test | |||
# TODO: Remove when https://github.com/element-fi/elf-contracts/issues/243 is fixed. | |||
sed -i 's|^\s*require(_expiration - block\.timestamp < _unitSeconds);\s*$||g' contracts/ConvergentCurvePool.sol | |||
|
|||
find . -name "*.sol" -exec sed -i -e 's/^\(\s*\)\(assembly\)/\1\/\/\/ @solidity memory-safe-assembly\n\1\2/' '{}' \; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one actually isn't memory-safe... and apparently I was fooled by our test cases in that just inlining unconditionally will just work and not break anything :-(.
7f1f87a
to
2775355
Compare
For the working external tests I get:
Unfortunately we don't get runtime gas stats for some yet... but interestingly code size also seems to generally decrease... But it still sucks a bit that it does still bring back more stack-too-deep cases. |
Impressive! |
409ea3c
to
2d52471
Compare
libyul/optimiser/FullInliner.cpp
Outdated
// Do not inline into already big functions. | ||
if (m_functionSizes.at(_callSite) > 45) | ||
if (m_functionSizes.at(_callSite) > (usesNewCodeTransform ? 450 : 45)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this version seems to fix all stack-too-deep errors in the external tests.
But I also have a hunch that #12132 might actually help a lot with stack pressure as well, so maybe on top of that we can even drop any boundary...
All the external failures here at this point seem to be due to NomicFoundation/hardhat#2453. I think it would be fine to just disable these tests until the issue is fixed. |
@ekpyron maybe fix the conflicts here so you can rebase and test the external contracts again? |
libyul/optimiser/FullInliner.cpp
Outdated
@@ -196,7 +205,7 @@ bool FullInliner::shallInline(FunctionCall const& _funCall, YulString _callSite) | |||
break; | |||
} | |||
|
|||
return (size < 6 || (constantArg && size < 12)); | |||
return (size < (usesNewCodeTransform ? 8 : 6) || (constantArg && size < (usesNewCodeTransform ? 16 : 12))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cameel Actually: tweaking this is also something to do here.
Just for reference, today's discussion with @ekpyron about what is still to be done here. The most important part:
|
6ea76c7
to
41faa38
Compare
41faa38
to
4c350b8
Compare
a6d231e
to
65695e3
Compare
ff71e3d
to
c592dcb
Compare
ed44ef0
to
826fc47
Compare
c592dcb
to
a59c0ce
Compare
826fc47
to
cc66991
Compare
a59c0ce
to
7b7409a
Compare
cc66991
to
52e796c
Compare
7b7409a
to
2817678
Compare
#12937 updated. Now all external tests should be passing here. |
52e796c
to
46eff4b
Compare
57e12cb
to
0fb13eb
Compare
46eff4b
to
49d5c0a
Compare
0fb13eb
to
77038aa
Compare
Updated benchmarks from 1070566.
|
project | bytecode_size | deployment_gas | method_gas |
---|---|---|---|
bleeps | -3.25% ✅ |
0% |
-0% |
brink | -7.5% ✅ |
||
colony | 0% |
||
elementfi | -2.45% ✅ |
||
ens | 0% |
-6.37% ✅ |
-0.64% ✅ |
euler | -2.52% ✅ |
-1.68% ✅ |
-2.07% ✅ |
gnosis | -3.83% ✅ |
-4.33% ✅ |
-0.02% ✅ |
perpetual-pools | -4.56% ✅ |
-2.66% ✅ |
-0.99% ✅ |
pool-together | -7.06% ✅ |
-5.58% ✅ |
-0.37% ✅ |
prb-math | -3.53% ✅ |
-3.48% ✅ |
0% |
trident | -9.78% ✅ |
-7.95% ✅ |
-6.25% ✅ |
uniswap | -4.28% ✅ |
-4.5% ✅ |
-1.24% ✅ |
yield_liquidator | -7.12% ✅ |
-6.1% ✅ |
-0.5% ✅ |
zeppelin | -4.75% ✅ |
-3.21% ✅ |
-0.18% ✅ |
Absolute
project | bytecode_size | deployment_gas | method_gas |
---|---|---|---|
bleeps | -4261 | 0 | -113 |
brink | -1018 | ||
colony | 0 | ||
elementfi | -14314 | ||
ens | 0 | -1984437 | -763921 |
euler | -4604 | -601676 | -62018944 |
gnosis | -2593 | -169903 | -11399 |
perpetual-pools | -9474 | -1107332 | -8007246 |
pool-together | -15248 | -2061198 | -234934 |
prb-math | -1341 | -285835 | 0 |
trident | -31030 | -2178611 | -27085001 |
uniswap | -7385 | -905462 | -33994790 |
yield_liquidator | -6306 | -931624 | -14831 |
zeppelin | -27540 | -3400500 | -770162 |
@@ -113,7 +113,7 @@ contract ERC20 { | |||
// ---- | |||
// constructor() | |||
// ~ emit Transfer(address,address,uint256): #0x00, #0x1212121212121212121212121212120000000012, 0x14 | |||
// gas irOptimized: 418388 | |||
// gas irOptimized: 364503 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, looks like UDVTs are getting a nice boost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, as it should be!
It's great that the bytecode size also improves for all external tests! |
Depends on #12937.