Simple inlining for old optimizer. #10761
Conversation
ekpyron
force-pushed
the
oldOptimizerInlining
branch
2 times, most recently
from
c9138ed to
6df56a8
Compare
ekpyron
force-pushed
the
oldOptimizerInlining
branch
from
6df56a8 to
19c5566
Compare
ekpyron
force-pushed
the
oldOptimizerInlining
branch
from
ab7f768 to
5ef8ce5
Compare
ekpyron
force-pushed
the
oldOptimizerInlining
branch
from
03d5ea3 to
906109e
Compare
ekpyron
force-pushed
the
oldOptimizerInlining
branch
4 times, most recently
from
bea6def to
55bb561
Compare
ekpyron
force-pushed
the
oldOptimizerInlining
branch
from
b3c4ead to
c3d83df
Compare
ekpyron
force-pushed
the
oldOptimizerInlining
branch
2 times, most recently
from
621235d to
374ca72
Compare
libevmasm/Inliner.cpp
Outdated
| ++block->pushTagCount; | ||
|
|
There was a problem hiding this comment.
I don't really follow how pushTagCount is used. Can you please explain?
There was a problem hiding this comment.
pushTagCount counts how many times the tag at which the respective inline candidate lives is pushed, i.e. roughly how many jumps there are into that block. When inlining we can increase the number of pushes, though, that's accounted for here.
Unlikely to happen currently, since we restrict to full function inlining, but more likely to happen in the more general case and still not impossible in full function inlining.
ekpyron
force-pushed
the
oldOptimizerInlining
branch
9 times, most recently
from
2f0eeb5 to
b4b7df8
Compare
|
FYI this is the difference in gas expectations https://gist.github.com/ekpyron/0b3c2b0ac499dfad578ad01ecdfd34ee stemming from rebasing this on top of #10863 (the diff is between updating once with the inliner disabled and once with it enabled). |
Changelog.md
Outdated
| @@ -4,6 +4,7 @@ Language Features: | |||
|
|
|||
|
|
|||
| Compiler Features: | |||
| * Optimizer: Simple inlining of functions with straight control flow. | |||
There was a problem hiding this comment.
Should we explain this in more depth in the documentation?
libevmasm/Inliner.cpp
Outdated
| { | ||
| // The number of PushTag's approximates the number of calls to a block. | ||
| if (item.type() == PushTag) | ||
| numPushTags[item.data()]++; |
There was a problem hiding this comment.
In general, do you handle dynamic references? Can you add some tests?
contract C {
function() internal pure returns (uint) x;
function f() internal pure returns (uint) { return 6; }
function g() public { x = f; }
function h() public returns (uint) { return x(); }
function i() public returns (uint) { return f(); } // this should be inlined
}
And another test where the assignment to x is done in the constructor and another where h and i are called in the constructor.
libevmasm/Inliner.cpp
Outdated
|
|
||
| // If the estimated runtime cost over the lifetime of the contract plus the deposit cost in the uninlined case | ||
| // exceed the inlined deposit costs, it is beneficial to inline. | ||
| if (bigint(m_runs) * uninlinedExecutionCost + uninlinedDepositCost > inlinedDepositCost) |
There was a problem hiding this comment.
Does this assume we can always delete the function if it is inlined?
There was a problem hiding this comment.
Yes... which is of course wrong in general... I could also calculate under the assumption that we can't remove it. It would probably also be possible to be sure about it (not only count pushtags, but also count which of those are pushtag jump), but only for "right now" and not for "maybe after further optimization cycles"...
There was a problem hiding this comment.
Can you take references from the surrounding assembly into account? That will probably not be removable since it is a dynamic jump.
There was a problem hiding this comment.
Yeah... let me see if I can... with "surrounding assembly" you mean the assembly the current assembly is a subassembly of, right :-)?
There was a problem hiding this comment.
Ah, ok, there is _tagsReferencedFromOutside available, I'll just have to pass that through, nice! I'll do that then!
libevmasm/Inliner.cpp
Outdated
| newItems += ranges::views::drop_last(inlinableBlock->items, 1); | ||
| newItems.emplace_back(*exitJump); | ||
|
|
||
| // We are removing one push tag to the block we inline. |
There was a problem hiding this comment.
Does this change the heuristic while are inlining?
There was a problem hiding this comment.
Yes... but only for a block that we already inlined once - and removing one jump to it will only make it more likely to inline it again afterwards, so probably fine...
Then again I could probably remove this and the loop below for now without any effect as well...
| stop | ||
| /* "optimizer_BlockDeDuplicator/input.sol":108:133 function fun_() public {} */ | ||
| tag_4: | ||
| jump(tag_3) |
There was a problem hiding this comment.
This could actually be replaced by a single stop...
There was a problem hiding this comment.
Yes - in general we can consider also inlining pushtag(tag) jump when we see tag: ... stop or tag: ... invalid or tag: ... revert... it'd need a different heuristics, though.
There was a problem hiding this comment.
Of course the size of the block to be inlined being smaller than the pushtag(tag) jump itself will always be good...
There was a problem hiding this comment.
I think it would be useful, even if again not that nice for the debugger. But maybe we can do that in another PR.
ekpyron
force-pushed
the
oldOptimizerInlining
branch
from
7d7aa7f to
dc0b255
Compare
|
I just updated this to split the tag data into subId and tag everywhere and to only ever consider tags referring to the local subassembly (I think we could never have inlined across subassemblys anyways and this way push counting and tag tracking should be more robust). I also pass in the externally referenced tags, s.t. we can consider them in the heuristics. Also I addressed most of the other comments I think, so mainly missing are tests and docs I hope. |
ekpyron
force-pushed
the
oldOptimizerInlining
branch
from
105930a to
670a1d8
Compare
ekpyron
force-pushed
the
oldOptimizerInlining
branch
from
c11f064 to
8222af9
Compare
|
Can you squash "fix windows build"? |
ekpyron
force-pushed
the
oldOptimizerInlining
branch
from
8222af9 to
e4f7cc4
Compare
ekpyron
force-pushed
the
oldOptimizerInlining
branch
from
e4f7cc4 to
53663f2
Compare
ekpyron
force-pushed
the
oldOptimizerInlining
branch
from
53663f2 to
cb74a45
Compare
|
@haltman-at @yann300 So as you can see we merged this yesterday, so tonight's nightly release should include this in case you want to have another look before it's released. The final version is restricted to only inline the "jump-in-jump-out" case, i.e. only entire functions will be inlined for the time being (we will extend this sooner or later, though, including to cases like #10761 (comment)). We decided to go ahead without first defining new debugging output, since we noticed that the other steps in the optimizer will already ignore the fact that jumps have jump labels and may remove jumps into- or out-of- functions already, so we're not introducing a new problem with this PR, but we're "just" making an existing problem worse - so given that and the fact that the gas savings of this can be significant and it is required for zero-cost workarounds for #10698, we thought it worthwhile to merge now. By now it seems clear that we want to define debugging data that specifies function entries and exits on a more fine-grained level, but it's not yet entirely clear what information we want exactly and in what form (Do we only want something equivalent to jump labels on opcode level? Do we also want information about the function entry being "complete", i.e. the arguments and return values being initialized on stack? In general what do we do if opcodes shared among several functions is collapsed? How best to try and make this robust against potential future optimizations? Etc.) - we have a forum topic around the topic of debugging information in general (see https://forum.soliditylang.org/t/debugging-information/77) and can hopefully come up with something soon. |
References #10698