SSA-CFG: Store function call arguments in the same order as Yul#16765
SSA-CFG: Store function call arguments in the same order as Yul#16765blishko wants to merge 1 commit into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Block0_0 [fillcolor="#FF746C", style=filled, label="\ | ||
| // Block 0; (0, max 0)\nLiveIn: \l\ | ||
| // LiveOut: \l\nUsed: \l\nv2 := f(0x02, 0x01)\l\ | ||
| // LiveOut: \l\nUsed: \l\nv2 := f(0x01, 0x02)\l\ |
There was a problem hiding this comment.
I think the previous output was incorrect, if you look at the yul source code, the function call is f(1,2).
| // Block 21 => v63\l\ | ||
| // )\l\ | ||
| // v6 := lt(v0, phi5)\l\ | ||
| // v6 := lt(phi4, v0)\l\ |
There was a problem hiding this comment.
Similarly here. The yul code is lt(x,a) where a is the function argument and corresponds to v0.
| // IN: [v3]\l\ | ||
| // \l\ | ||
| // [lit7, v3]\l\ | ||
| // [v3, lit7]\l\ |
There was a problem hiding this comment.
This is interesting. I am not sure why the arguments to eq are swapped here.
In any case, it gave me an idea how we can take advantage of commutative operations. I made a note about that in our document.
There was a problem hiding this comment.
This was the result of how switch statement is handled in SSACFGBuilder.
The builder basically treats it as a sequence of nested if-then-else and it generates eq instructions for that purpose.
In order to preserve the previous behaviour, we need to swap the arguments passed to eq.
This has been done now.
blishko
force-pushed
the
ssa-change-arg-order
branch
from
b93deda to
5c5088a
Compare
| for (auto&& [idx, arg]: _call.arguments | ranges::views::enumerate | ranges::views::reverse) | ||
| for (auto&& [idx, arg]: _call.arguments | ranges::views::enumerate) | ||
| if (!builtin.literalArgument(idx).has_value()) | ||
| inputs.emplace_back(std::visit(*this, arg)); |
There was a problem hiding this comment.
This is a bit surprising as we're iterating over yul ast arguments. was this just wrong before?
There was a problem hiding this comment.
Nice catch!
This needs to stay the same. Yul arguments are evaluated from right to left (https://docs.soliditylang.org/en/v0.8.35/yul.html#function-calls) so we need to visit them in that order.
blishko
force-pushed
the
ssa-change-arg-order
branch
2 times, most recently
from
77a32a4 to
767d010
Compare
In SSA-CFG we were previously storing function call arguments in reversed order compared to Yul and EVM instructions. The reversed order is more natural for stack shuffler, but it can cause confusion in order places. We should use the same order as Yul and EVM. Since we now visit the arguments in different order, the order of operations in a basic block can change and this has effects down the line. For example, the stack layout might be different and eventually also the bytecode can differ.
blishko
force-pushed
the
ssa-change-arg-order
branch
from
767d010 to
e2f0066
Compare
|
I believe this is now ready! |
2 participants
In SSA-CFG we were previously storing function call arguments in reversed order compared to Yul and EVM instructions. The reversed order is more natural for stack shuffler, but it can cause confusion in order places. We should use the same order as Yul and EVM.
Since we now visit the arguments in different order, the order of operations in a basic block can change and this has effects down the line. For example, the stack layout might be different and eventually also the bytecode can differ.