Handle "leaf functions" first in full inliner. #9314
Conversation
| @@ -508,54 +508,52 @@ | |||
| // for { } lt(i, length) { i := add(i, 1) } | |||
| // { | |||
| // if iszero(slt(add(src, _1), end)) { revert(0, 0) } | |||
| // let _4 := 0x2 | |||
| // let dst_1 := allocateMemory(array_allocation_size_t_array$_t_uint256_$2_memory(_4)) | |||
| // let dst_1 := allocateMemory(_3) | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
We should compile some contracts via yul and perform gas checks. |
It would be really nice if we had a benchmark for optimization rules. What do we currently do? |
|
There are some very limited gas tests, we should add some more, especially also comparing compiling via yul not! |
| handleBlock(fun.second->name, fun.second->body); | ||
| updateCodeSize(*fun.second); | ||
| vector<YulString> removed; | ||
| for (auto it = cg.functionCalls.begin(); it != cg.functionCalls.end();) |
There was a problem hiding this comment.
We should not iterate in the YulString order.
There was a problem hiding this comment.
Please check but I think it is not a problem anymore.
|
To make the impact of renaming functions as small as possible, this should use |
chriseth
force-pushed
the
inlineStartingFromLeaves
branch
5 times, most recently
from
fdca159 to
f230638
Compare
| // } | ||
| // function f() | ||
| // function f_23_88(a) -> x |
There was a problem hiding this comment.
Looks like this does not work anymore.
| // } | ||
| // function foo(b) -> out1, out2 | ||
| // function foo_24_75(a, b) -> out1, out2, out3 |
| // f() | ||
| // sstore(0, 1) | ||
| // } | ||
| // function f() |
There was a problem hiding this comment.
Is this test still valid?
test/libyul/yulOptimizerTests/fullSuite/unusedFunctionParameterPruner.yul
Outdated
Show resolved
Hide resolved
libyul/optimiser/FullInliner.cpp
Outdated
| while (true) | ||
| { | ||
| vector<YulString> removed; | ||
| for (auto it = cg.functionCalls.begin(); it != cg.functionCalls.end();) | ||
| { | ||
| auto const& [fun, callees] = *it; | ||
| if (callees.empty()) | ||
| { | ||
| removed.emplace_back(fun); | ||
| depths[fun] = currentDepth; | ||
| it = cg.functionCalls.erase(it); | ||
| } | ||
| else | ||
| ++it; | ||
| } | ||
|
|
||
| for (auto& call: cg.functionCalls) | ||
| call.second -= removed; | ||
|
|
||
| currentDepth++; | ||
|
|
||
| if (removed.empty()) | ||
| break; | ||
| } |
There was a problem hiding this comment.
Assuming that an empty set (for CallGraph map's .second) corresponds to say a starting node s. And if we invert the map, then the problem of depth finding gets reduced to finding the longest path to all vertices starting from s. I think this may be solved easier. (Abstractly this is just putting all edge weights -1 and finding shortest paths from s, but there might be a simple way to do it.)
There was a problem hiding this comment.
Can you elaborate on this a little? It currently does not sound simpler to me :)
There was a problem hiding this comment.
What I meant is the following. Construct a directed graph where each vertex is a FunctionDefinition. There is an edge from a -> b if b calls a (this is same as the "inverted map" of CallGraph) Define an extra vertex s and extra edges from s to a vertex v if v does not call any other function (i.e., indegree of v is zero)
Let's assign weight -1 to all the edges. Now, the problem of finding depth is the same as finding the shortest path from s to all vertices. Theoretically, Bellman-Ford should solve it. Maybe there is a simpler way since all the weights are the same.
But I guess the current implementation is fine too.
Edit: recursion/cycles will make bellman-ford not work for our case :(.
| // totalCost: 615251 | ||
| // codeDepositCost: 597000 | ||
| // executionCost: 632 | ||
| // totalCost: 597632 |
There was a problem hiding this comment.
A diff of the changes. Some functions with unused function parameters exist, so we can further decrease the costs. But still the gas costs are lower than this point
| } | ||
| { | ||
| let _3, _4 := storage_array_index_access_t_array$_t_uint256_$dyn_storage(_1, vloc_i) |
There was a problem hiding this comment.
The sload(_1) that comes from here is gone. So this is a nice change.
chriseth
force-pushed
the
inlineStartingFromLeaves
branch
2 times, most recently
from
e2a0d8d to
cb8925e
Compare
chriseth
force-pushed
the
inlineStartingFromLeaves
branch
from
cb8925e to
42c26e0
Compare
|
Rebased. |
|
@hrkrshnn your last commit looks good. Is this ready to be merged then? :) |
Fixes #9127