Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Yul: Introduces ASTNodeRegistry #15823

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

clonker
Copy link
Member

@clonker clonker commented Feb 4, 2025

This PR supersedes PR #15242. Depends on PR #15821.

It introduces yul::ASTNodeRegistryBuilder and yul::ASTNodeRegistry. Most important bits of the API:

┌──────────────────────────────────────────────────────────────────────────┐                                      
│ASTNodeRegistryBuilder                                                    │                                      
├──────────────────────────────────────────────────────────────────────────┤                                      
│ ASTNodeRegistryBuilder()                                                 │                                      
│ explicit ASTNodeRegistryBuilder(ASTNodeRegistry const& _existingRegistry)│                                      
│                                                                          │                                      
│ ASTNodeRegistry::NodeIdidefine(std::string_view _label)                  │                                      
│ ASTNodeRegistry build() const                                            │                                      
└────────────────────────────────────┬─────────────────────────────────────┘                                      
                                     │                                                                            
                                     │                                                                            
                                     │                                                                            
┌────────────────────────────────────┴─────────────────────────────────────────────────────────────────────────┐  
│ASTNodeRegistry                                                                                               │  
├──────────────────────────────────────────────────────────────────────────────────────────────────────────────┤  
│ ASTNodeRegistry()                                                                                            │  
│ ASTNodeRegistry(std::vector<std::string> const& _labels, std::vector<size_t> const& _nameToLabelMapping)     │  
│                                                                                                              │  
│ std::string_view operator()(NodeIde_id)                                                                      │  
│ std::optional<YulName> findNameForLabel(std::string_view _label) const                                       │  
└────────────────────────────────────┬─────────────────────────────────────────────────────────────────────────┘  
                                     │                                                                            
                                     │                                                                            
                                     │                                                                            
┌────────────────────────────────────┴───────────────────────────────────────────────────────────────────────────┐
│NodeIdDispenser                                                                                                 │
├────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ explicit NodeIdDispenser(ASTNodeRegistry const& _existingRegistry, std::set<std::string> const& _reserved = {})│
│                                                                                                                │
│ NodeId newId(NodeId _parent = 0)                                                                               │
│ NodeId newGhost()                                                                                              │
│ ASTNodeRegistry generate(Block const& _root, Dialect const& _dialect) const;                                   │
└────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

I have not yet included the NodeIdDispenser, this can be done in a separate PR.

The ASTNodeRegistry lives (immutably) in the AST and can be used to query labels for particular nodes in the AST. But potentially this could store more metadata than just string labels.

A ASTNodeRegistry can be created by using the ASTNodeRegistryBuilder - which would be the preferred way when importing or parsing (see PR #15833):

NameWithDebugData AsmJsonImporter::createNameWithDebugData(Json const& _node)
{
auto nameWithDebugData = createAsmNode<NameWithDebugData>(_node);
nameWithDebugData.name = m_nodeLabelRegistryBuilder.define(member(_node, "name").get<std::string>());
return nameWithDebugData;
}

AST AsmJsonImporter::createAST(solidity::Json const& _node)
{
auto block = createBlock(_node);
return {m_dialect, m_nodeLabelRegistryBuilder.build(), std::move(block)};
}

Or, during/after optimization, from the NodeIdDispenser based on the root block, discarding everything that is not part of the current AST, and the dialect to check for collisions with reserved identifiers:

_object.setCode(std::make_shared<AST>(dialect, nameDispenser, std::move(astRoot)));

solidity/libyul/AST.cpp

Lines 110 to 114 in c2cea37

AST::AST(Dialect const& _dialect, NodeIdDispenser const& _nameDispenser, Block _root):
m_dialect(_dialect),
m_labels(_nameDispenser.generateNewLabels(_root, _dialect)),
m_root(std::move(_root))
{}

@clonker clonker added the has dependencies The PR depends on other PRs that must be merged first label Feb 4, 2025
@clonker clonker requested a review from cameel February 4, 2025 11:03
@clonker clonker force-pushed the add_yul_name_label_registry branch from d252184 to cd00616 Compare February 4, 2025 11:25
Base automatically changed from string_vew_compat to develop February 4, 2025 13:41
@clonker clonker removed the has dependencies The PR depends on other PRs that must be merged first label Feb 4, 2025
@clonker clonker requested review from r0qs and aarlt February 5, 2025 09:14
Copy link
Contributor

@blishko blishko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except the minor comments, it seems OK to me.

libyul/ASTNodeRegistry.cpp Outdated Show resolved Hide resolved
libyul/ASTNodeRegistry.cpp Outdated Show resolved Hide resolved
@clonker clonker force-pushed the add_yul_name_label_registry branch from cd00616 to 2b9295d Compare February 5, 2025 12:10
@blishko
Copy link
Contributor

blishko commented Feb 5, 2025

@clonker, BTW, one thing that I found confusing was referring to things related to type NodeId as names. Not sure if this is because of YulName, and if this is what you also pointed out as annoying on Monday.
Just saying for the record :)

@clonker clonker force-pushed the add_yul_name_label_registry branch from 2b9295d to bf80a01 Compare February 5, 2025 12:51
@clonker
Copy link
Member Author

clonker commented Feb 5, 2025

@clonker, BTW, one thing that I found confusing was referring to things related to type NodeId as names. Not sure if this is because of YulName, and if this is what you also pointed out as annoying on Monday. Just saying for the record :)

You are right and this is what I was referring to on Monday :) I have made things self-consistent now as in everything is "id" and there is no more mention of "name". Change on the larger scope (as in changing YulName to something else) is pending/to be discussed. Thanks for pointing it out!

Copy link
Contributor

@blishko blishko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!
But my review is rather superficial, I don't have enough knowledge about the design and how this is gonna be used.
Someone else should also have a look at this as well :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants