Yul: Introduces ASTNodeRegistry #15823

clonker · 2025-02-04T10:59:27Z

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):

solidity/libyul/AsmJsonImporter.cpp

Lines 82 to 87 in c2cea37

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

solidity/libyul/AsmJsonImporter.cpp

Lines 56 to 60 in c2cea37

    
           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:

solidity/libyul/optimiser/Suite.cpp

Line 200 in c2cea37

    
           _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)) 
        
           {}

blishko

Except the minor comments, it seems OK to me.

libyul/ASTNodeRegistry.cpp

blishko · 2025-02-05T12:42:56Z

@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 · 2025-02-05T12:52:31Z

@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!

blishko

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 :)

cameel

Not a complete review yet, just a few things I noticed so far.

libyul/ASTNodeRegistry.cpp

libyul/ASTNodeRegistry.h

cameel · 2025-02-10T13:34:32Z

libyul/ASTNodeRegistry.h

+namespace solidity::yul
+{
+
+class ASTNodeRegistry


You described these classes in the PR description, but I'd much rather just have docstrings for them :)

Especially m_idToLabelMapping and m_labels should be documented. Like, what are the numbers stored in m_idToLabelMapping and the assumptions about them (can there be duplicates? do both vectors have to have same length?)

I have added some docs - let me know what you think and/or if it should be expanded somewhere. :)

One thing I still don't fully understand is the purpose of the ghost IDs. Looking at other PRs, I see the mechanism for adding them, but not yet any place that would actually add them. The intended usage is a detail that the docstring should explain.

Ghost nodes are added during CFG construction. They only live in the CFG itself and are not actually referenced in the AST. We could potentially also remove them here and specialize them out for CFGs. Then it's more local to where they are introduced and needed.

Well, yeah, their presence here threw me off because I initially thought you needed them for something in the AST and the naming from CFG was only a coincidence. So might be clearer to separate those.

On the other hand, now that it's properly documented and it's clear why we need it, I'd also be fine with leaving it here. It leaks CFG implementation details a bit, but I imagine that separating this out might complicate things.

Yeah it would complicate things to completely separate it out. But it does work well to let the CFG render ghost labels so at least that detail doesn't leak.

libyul/ASTNodeRegistry.cpp

libyul/ASTNodeRegistry.h

cameel · 2025-02-11T15:50:59Z

libyul/ASTNodeRegistry.h

+	/// unique labels in the container, beginning with empty ("") and ghost (ghostPlaceholder).
+	std::vector<std::string> m_labels;
+	/// Each element in the vector is one NodeId. The value of the vector points to the corresponding label. E.g.,
+	/// m_labels[m_idToLabelMapping[3]] is the label for NodeId 3. Therefore, there can be duplicates and the lengths
+	/// of `m_labels` and `m_idToLabelMapping` do not need to correspond.
+	std::vector<size_t> m_idToLabelMapping;
+	mutable std::map<NodeId, std::string> m_ghostLabelCache;


Suggested change

/// unique labels in the container, beginning with empty ("") and ghost (ghostPlaceholder).

std::vector<std::string> m_labels;

/// Each element in the vector is one NodeId. The value of the vector points to the corresponding label. E.g.,

/// m_labels[m_idToLabelMapping[3]] is the label for NodeId 3. Therefore, there can be duplicates and the lengths

/// of `m_labels` and `m_idToLabelMapping` do not need to correspond.

std::vector<size_t> m_idToLabelMapping;

mutable std::map<NodeId, std::string> m_ghostLabelCache;

/// Unique labels in the container; the first two items are: empty ("") and ghost (ghostPlaceholder).

std::vector<std::string> m_labels;

/// Each index in the vector is one NodeId. The value of the vector points to the corresponding label. E.g.,

/// m_labels[m_idToLabelMapping[3]] is the label for NodeId 3. Therefore, there can be duplicates and the lengths

/// of `m_labels` and `m_idToLabelMapping` do not need to correspond.

std::vector<size_t> m_idToLabelMapping;

mutable std::map<NodeId, std::string> m_ghostLabelCache;

Though TBH this still does not say everything I wanted to know when I started reviewing this. That's how I'd put it myself:

Suggested change

/// unique labels in the container, beginning with empty ("") and ghost (ghostPlaceholder).

std::vector<std::string> m_labels;

/// Each element in the vector is one NodeId. The value of the vector points to the corresponding label. E.g.,

/// m_labels[m_idToLabelMapping[3]] is the label for NodeId 3. Therefore, there can be duplicates and the lengths

/// of `m_labels` and `m_idToLabelMapping` do not need to correspond.

std::vector<size_t> m_idToLabelMapping;

mutable std::map<NodeId, std::string> m_ghostLabelCache;

/// All Yul AST node labels present in the registry.

/// Always contains at least two items: an empty label and a ghost placeholder.

/// All items must be unique. All but the first two must be valid Yul identifiers.

std::vector<std::string> m_labels;

/// Assignment of labels to `NodeId`s. Indices are `NodeId`s and values are indices into `m_labels`.

/// Every label except ghost placeholder always has exactly one `NodeId` pointing at it.

/// Ghost placeholder can have more than one.

std::vector<size_t> m_idToLabelMapping;

/// Artificial labels generated for ghost IDs from a template.

/// Generated on demand through `lookupGhost()` and cached for future lookups.

/// Must never contain non-ghost IDs. Labels are guaranteed to be unique.

mutable std::map<NodeId, std::string> m_ghostLabelCache;

But note that it includes some of my assumptions how it should work, which may or may not be true depending on the answers to my earlier comments.

BTW, I think we should make m_labels and m_idToLabelMapping const. The container is immutable so they're not supposed to ever change after initialization.

BTW, I think we should make m_labels and m_idToLabelMapping const. The container is immutable so they're not supposed to ever change after initialization.

That'll get rid of implicit copy/move, though. The immutability is reflected by it not having any non-const methods. That would already take care of that unless one const-casts.

But won't that be just for assignments and still give us constructors? I mean, assigning the registry in place kinda violates its immutability so not having them actually makes sense.

In any case, not a big deal, we could leave them non-const, but I still don't see why it wouldn't make sense :)

We can revisit that once the rest is in place, there are some cases in which not being able to assign it would require additional refactoring because of order of initialization and dependencies. If it's fine for you I'd leave it non-const for now.

Sure.

TBH I don't mind this not being const as much as the fact that we're going rely on overwriting whole registries as a workaround for initialization order. That sounds like it could backfire :)

Yeahh... Made them const after all. :)

cameel · 2025-02-11T15:55:36Z

libyul/ASTNodeRegistry.h

+namespace solidity::yul
+{
+
+class ASTNodeRegistry


One thing I still don't fully understand is the purpose of the ghost IDs. Looking at other PRs, I see the mechanism for adding them, but not yet any place that would actually add them. The intended usage is a detail that the docstring should explain.

libyul/ASTNodeRegistry.cpp

libyul/ASTNodeRegistry.h

cameel

I think we have most things ironed out, the most important remaining one being the need for text representation for ghost labels. Also docs/naming for those "unused" labels.

Other than that I have some smaller nitpicks from the final pass trough the code.

After that we'll be done here.

libyul/ASTLabelRegistry.h

libyul/ASTLabelRegistry.cpp

libyul/ASTLabelRegistry.h

cameel

Going through the PR again I found some outdated bits that need to be cleaned up before we merge (like mentions of the now non-existent ghost placeholder or Id instead of ID), but I have no more issues with how it's implemented so I'm already approving.

libyul/ASTLabelRegistry.cpp

libyul/ASTLabelRegistry.h

cameel · 2025-02-14T22:06:20Z

libyul/ASTLabelRegistry.h

+
+	ASTLabelRegistry::LabelID define(std::string_view _label);
+	ASTLabelRegistry::LabelID addGhost();
+	ASTLabelRegistry build() const;


As mentioned in #15823 (comment), please add a docstring explaining what gets copied. In particular that the unused labels do not carry over, but ghosts do.

Added such docstring. Unused label ids do carry over, as the m_nextID is based on the existing registries' maxID - which includes them.

libyul/ASTLabelRegistry.cpp

clonker added the has dependencies The PR depends on other PRs that must be merged first label Feb 4, 2025

clonker mentioned this pull request Feb 4, 2025

Introduce YulNameRepository #15242

Closed

clonker requested a review from cameel February 4, 2025 11:03

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 removed the has dependencies The PR depends on other PRs that must be merged first label Feb 4, 2025

clonker requested review from r0qs and aarlt February 5, 2025 09:14

clonker mentioned this pull request Feb 5, 2025

Add ASTNodeRegistry to AST #15833

Open

blishko reviewed Feb 5, 2025

View reviewed changes

libyul/ASTNodeRegistry.cpp Outdated Show resolved Hide resolved

libyul/ASTNodeRegistry.cpp Outdated Show resolved Hide resolved

clonker force-pushed the add_yul_name_label_registry branch from cd00616 to 2b9295d Compare February 5, 2025 12:10

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

blishko reviewed Feb 5, 2025

View reviewed changes

clonker mentioned this pull request Feb 7, 2025

Add id dispenser for numerical yul node ids #15838

Open

cameel reviewed Feb 10, 2025

View reviewed changes

clonker force-pushed the add_yul_name_label_registry branch 2 times, most recently from 03abf50 to af57ab2 Compare February 10, 2025 15:45

ethereum deleted a comment from stackenbotten Feb 10, 2025

clonker force-pushed the add_yul_name_label_registry branch 2 times, most recently from 5ade700 to 4602bf7 Compare February 10, 2025 15:49

cameel reviewed Feb 11, 2025

View reviewed changes

clonker force-pushed the add_yul_name_label_registry branch 6 times, most recently from 837871f to 43a00c6 Compare February 13, 2025 11:22

cameel reviewed Feb 13, 2025

View reviewed changes

clonker force-pushed the add_yul_name_label_registry branch 3 times, most recently from b0f5c6b to 2d04f51 Compare February 14, 2025 14:18

cameel previously approved these changes Feb 14, 2025

View reviewed changes

clonker dismissed cameel’s stale review via 77c2bb0 February 19, 2025 12:26

clonker force-pushed the add_yul_name_label_registry branch 2 times, most recently from 77c2bb0 to 44cace1 Compare February 19, 2025 12:27

Yul: Introduces ASTNodeRegistry

f330e45

clonker force-pushed the add_yul_name_label_registry branch from 44cace1 to f330e45 Compare February 19, 2025 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yul: Introduces ASTNodeRegistry #15823

Yul: Introduces ASTNodeRegistry #15823

clonker commented Feb 4, 2025 •

edited

Loading

blishko left a comment

blishko commented Feb 5, 2025

clonker commented Feb 5, 2025

blishko left a comment

cameel left a comment

cameel Feb 10, 2025

cameel Feb 10, 2025

clonker Feb 10, 2025

cameel Feb 11, 2025

clonker Feb 12, 2025 •

edited

Loading

cameel Feb 13, 2025

clonker Feb 14, 2025

cameel Feb 11, 2025

cameel Feb 11, 2025

clonker Feb 12, 2025

cameel Feb 13, 2025 •

edited

Loading

clonker Feb 14, 2025

cameel Feb 14, 2025

clonker Feb 19, 2025

cameel Feb 11, 2025

cameel left a comment

cameel left a comment

cameel Feb 14, 2025

clonker Feb 19, 2025

	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)};
	}

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

Yul: Introduces ASTNodeRegistry #15823

Are you sure you want to change the base?

Yul: Introduces ASTNodeRegistry #15823

Conversation

clonker commented Feb 4, 2025 • edited Loading

blishko left a comment

Choose a reason for hiding this comment

blishko commented Feb 5, 2025

clonker commented Feb 5, 2025

blishko left a comment

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clonker Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clonker commented Feb 4, 2025 •

edited

Loading

clonker Feb 12, 2025 •

edited

Loading

cameel Feb 13, 2025 •

edited

Loading