replace YulString with YulName typedef #15083
Conversation
clonker
force-pushed
the
rename_yulstring_to_yulname
branch
from
378b951
to
fdb537b
Compare
…so that it can be replaced more easily.
clonker
force-pushed
the
rename_yulstring_to_yulname
branch
from
fdb537b
to
1e91d0e
Compare
|
What's this about? Is this regarding the Yul AST rework to get rid of the AST ID dependencies (or to at least strengthen them) in order then get rid of codegen non-determinism? Or is it something else? |
|
The idea is to get rid of YulString as a kind of a leightweight string proxy in favor of a simple incremental uint64 index for handles (with a backing string vector of names) and do the string generation of derived names post-hoc |
Alright, so killing the non-determinism in its root. |
|
Depends on the level at which you're looking - but sure, restarting the optimization with an intermediate will kill determinism with that approach unless we do reordering after each step (which may not be so bad, let's see) |
|
Hi @bshastry, as this changes some stuff in the fuzzing department as well, does the PR mess things up on your end or is it fine? :) |
| using TypedNameList = std::vector<TypedName>; | ||
|
|
||
| /// Literal number or string (up to 32 bytes) | ||
| enum class LiteralKind { Number, Boolean, String }; | ||
| struct Literal { langutil::DebugData::ConstPtr debugData; LiteralKind kind; YulString value; Type type; }; | ||
| struct Literal { langutil::DebugData::ConstPtr debugData; LiteralKind kind; YulName value; Type type; }; |
There was a problem hiding this comment.
The PR seems to be just replacing all instances of YulString with YulName. That's not very useful. The idea was to rename only in those cases where YulString is used for identifiers. Those are the cases which we'd want to replace with numeric IDs and we'll do this by switching YulName to be some other type.
Literal is a good example of a situation where this must not happen. If you have a string "xyz", in the code, it must remain as "xyz" in the AST. It can't be replaced with some number that we'll only later remap back to the original value.
I think we don't want this for TypedName either. Types are defined in the dialect so renaming then just in the AST will make us unable to find the right type definition.
BTW, TypeName is probably a legacy thing at this point and maybe we'll remove it eventually. It is used in the typed Yul dialect, which was an intermediate step in the process of translating the code to Ewasm. Ewasm was dropped and I'm not sure if we'll have any other use for it. In the untyped dialect Type will always be just the default type.
As for FunctionDefinition, for that one using YulName seems correct, though I'm surprised it doesn't use a whole Identifier instead.
There was a problem hiding this comment.
I was under the impression you'd wanted to get rid of YulString altogether, my bad then.
As for Literal and TypedName: How would replacing a string "xyz" with some identifier and later remapping it be conceptually different from using YulString, which replaces the string "xyz" with a handle (id/hash) to later remap it?
| @@ -59,7 +60,7 @@ struct VariableDeclaration { langutil::DebugData::ConstPtr debugData; TypedNameL | |||
| /// Block that creates a scope (frees declared stack variables) | |||
| struct Block { langutil::DebugData::ConstPtr debugData; std::vector<Statement> statements; }; | |||
| /// Function definition ("function f(a, b) -> (d, e) { ... }") | |||
| struct FunctionDefinition { langutil::DebugData::ConstPtr debugData; YulString name; TypedNameList parameters; TypedNameList returnVariables; Block body; }; | |||
| struct FunctionDefinition { langutil::DebugData::ConstPtr debugData; YulName name; TypedNameList parameters; TypedNameList returnVariables; Block body; }; | |||
There was a problem hiding this comment.
We may have a small problem here. With this change, user-defined functions will use YulName, while the built-in functions in the dialect will still be identified with YulString. Since the latter by design have fixed names, I'm not sure it's even feasible to use YulName for them. But then FunctionCall node will not be able to refer to them.
| { | ||
|
|
||
| class YulString; | ||
| using YulName = YulString; |
There was a problem hiding this comment.
Unless you're planning to add something more here, like utilities (or maybe make YulName into a class in the future), it would be enough to put it in AST.h. We'll need it pretty much only in Yul AST. In fact, if there is some situation where it does not seem appropriate to include the whole AST.h, it could be a sign that YulString there is not really used for identifiers.
in preparation of type change of YulString s.t. these changes are more localized