[yul] Functions: Remove dependency on AST ID. #11512
Conversation
|
@chriseth did you mean to fix this issue like this? The current implementation seem to have a problem with structs and mappings. I didn't find the reason yet. |
|
Yes, this is what I meant, but I'm still not sure if it does not create more problems. |
|
How is this solving the core problem? Isn't this just reducing the probability? As long as the order of evaluation matches (even if AST ids are different), this will produce identical results. How likely is it that the AST ids change, but the order of evaluation remains? |
|
It at least condenses the IDs. |
There was a problem hiding this comment.
Semantic tests are failing due to wrong functions being called via pointers so you must have missed one of the .id() calls somewhere.
What about immutable IDs? There's one place in IRGeneratorForStatements that uses AST IDs for them:
define(result) << "loadimmutable(\"" << to_string(_immutable.variable->id()) << "\")\n";As to the PR overall, looks like it makes the IDs local to the contract being compiled and dependent only on visiting order, which should be stable against adding/removing/reordering input files so I think it's a step in a good direction.
| @@ -186,6 +192,8 @@ class IRGenerationContext | |||
| /// the code contains a call via a pointer even though a specific function is never assigned to it. | |||
| /// It will fail at runtime but the code must still compile. | |||
| InternalDispatchMap m_internalDispatchMap; | |||
| std::map<int64_t, int64_t> m_functionIds; | |||
There was a problem hiding this comment.
This needs a docstring describing what is the key and what is the value.
| @@ -150,6 +150,12 @@ class IRGenerationContext | |||
| bool inlineAssemblySeen() const { return m_inlineAssemblySeen; } | |||
| void setInlineAssemblySeen() { m_inlineAssemblySeen = true; } | |||
|
|
|||
| int64_t functionId(int64_t _astId) { | |||
| if (m_functionIds.find(_astId) == m_functionIds.end()) | |||
There was a problem hiding this comment.
generateInternalDispatchFunctions() rejects functions with ID 0:
// 0 is reserved for uninitialized function pointers
solAssert(function->id() != 0, "Unexpected function ID: 0");Maybe this assert should be moved (or copied) here?
I think that the biggest problem with IDs was that they change when you compile a different set of input files. E.g. you'd get different IDs when compiling |
|
I'm now more confident that the IDs generated here are pretty stable. The function generation queue is based on AST IDs, the rest of the code generation is more or less in source order (depends on how sub-nodes are visited), and function IDs are generated when they are first requested, i.e. when a function is referenced. |
|
This does create a disparity between the AST IDs that are part of the generated yul code (the function names) and the actual values used at runtime. Is that a problem? We have to use AST IDs for function names because we also generate yul functions for external solidity functions or functions that are not referenced and these AST IDs should be stripped in the final IR code anyway. |
chriseth
force-pushed
the
issue_10342
branch
2 times, most recently
from
3e09ab6
to
ed51f34
Compare
chriseth
force-pushed
the
issue_10342
branch
3 times, most recently
from
b0788fe
to
c6d2d10
Compare
| @@ -32,6 +32,6 @@ contract C { | |||
| // compileViaYul: also | |||
| // ---- | |||
| // f() -> 42, 23, 34, 42, 42 | |||
| // gas irOptimized: 111210 | |||
There was a problem hiding this comment.
Haha nice, this reduction is because a PUSH1 87 is replaced by a PUSH1 1 :)
There was a problem hiding this comment.
Here's the diff: https://gist.github.com/hrkrshnn/03381b12959e8f5709a6aadff7cd69da
The difference is from inlining.
Previously, the code changes once you add another source file to the inputs, even if the source file is fully unrelated to the contract being complied. Now, the code only changes if the current contract references a new internal function for use as a function pointer, so it should be fully independent from "cosmetic" source code changes or compiling all files at the same time / only compiling a single file. |
|
Renamed the function. |
Fixes #10342.