Add id dispenser for numerical yul node ids #15838

clonker · 2025-02-07T11:06:02Z

~~Depends on #15823~~

clonker · 2025-02-07T11:07:19Z

libyul/optimiser/NodeIdDispenser.cpp

+	// this can be replaced by the actually used ids in the provided block once the AST uses ids instead of YulString
+	std::set<NodeId> usedIds = ranges::views::iota(static_cast<size_t>(0), m_mapping.size() + m_offset) | ranges::to<std::set<NodeId>>;


Version where AST contains numerical ids:

solidity/libyul/optimiser/NodeIdDispenser.cpp

Lines 67 to 71 in 9372af9

std::set<NodeId> usedIds = NameCollector(_root).names();

// add ghosts to usedIds as they're not referenced in the regular ast

for (size_t i = 0; i < m_mapping.size(); ++i)

if (m_mapping[i] == ASTNodeRegistry::ghostId())

usedIds.insert(i + m_offset);

libyul/optimiser/NodeIdDispenser.h

libyul/optimiser/OptimizerUtilities.h

cameel · 2025-03-07T20:37:01Z

libyul/optimiser/LabelIDDispenser.h

+	/// Reserved labels, equipped with the transparent less comparison operator to be able to handle string_view.
+	std::set<std::string, std::less<>> m_reservedLabels;
+	/// Offset by which LabelIDs must be shifted to be used in the dispenser's `m_idToLabelMapping`
+	size_t m_offset;


I wanted to suggest a clearer name for this, but actually I think it would it be much more self-explanatory to just use ASTLabelRegistry::maxID() directly.

Oh yeah, absolutely. Thanks for the suggestion!

cameel · 2025-03-07T20:47:26Z

libyul/optimiser/LabelIDDispenser.h

+	ASTLabelRegistry const& labels() const { return m_labels; }
+
+	/// Spawns a new LabelID which depends on a parent LabelID that will be used for its string representation.
+	LabelID newID(LabelID _parent = 0);


The docstring should say that the parent label must not be an unused one.

Same for resolveParentLabelID().

And for generateNewLabels() we should mention that the resulting registry will never contain any unused IDs. They will always have new labels generated.

And for generateNewLabels() we should mention that the resulting registry will never contain any unused IDs. They will always have new labels generated.

It will have unused IDs. Right now I am not compressing the id range but it's append-only. As ids grow, so does the number of unused ones in the lower ranges. I thought about compressing them to the used ones, which would mean potentially remapping existing IDs in the AST and make generateNewLabels return a fresh AST. We could still do that if we figure it's a performance bottleneck or we waste too much memory. But personally I don't think so.

cameel · 2025-03-07T20:57:21Z

libyul/optimiser/LabelIDDispenser.cpp

+bool LabelIDDispenser::ghost(LabelID const _id) const
+{
+	yulAssert(_id < m_idToLabelMapping.size() + m_offset, "ID exceeds bounds.");
+	if (_id >= m_offset)
+		return m_idToLabelMapping[_id - m_offset] == ASTLabelRegistry::ghostLabelIndex();
+
+	return m_labels.ghost(_id);
+}


The function will return false if _id's parent is a ghost. Is that intentional? If not, then we should assert against that possibility in newID(). Note that currently newID() only rejects IDs which are ghosts themselves (due to range assert in resolveParentLabelID()).

newID()'s docstring should also document how it handles ghosts in either form.

Yeah in my mind one should only use newGhost to generate new ghosts. Deriving labels from ghosts has the potential to break things in an intricate fashion. Definitely not intended. I'm adding an assert + documentation of this.

cameel · 2025-03-07T20:59:17Z

libyul/optimiser/LabelIDDispenser.cpp

+}
+
+
+ASTLabelRegistry LabelIDDispenser::generateNewLabels(Block const&, Dialect const& _dialect) const


If we can't use the _block yet, IMO it would be best not add this parameter for now. I'd rather see the places that need to pass it in modified in the same PR that will implement the functionality.

That is fair:) I thought it would be easier this way but if you prefer it without the parameter then I'll remove it for now.

cameel · 2025-03-07T21:07:55Z

libyul/optimiser/LabelIDDispenser.cpp

+
+#include <libyul/optimiser/LabelIDDispenser.h>
+
+#include <libyul/optimiser/NameCollector.h>


The class does not really seem to use anything from NameCollector.h. Or did I miss something?

Didn't miss anything, it's in the same vein as the block argument: to be used once fully integrated. I'll remove the include for now.

cameel · 2025-03-07T21:23:59Z

libyul/optimiser/LabelIDDispenser.cpp

+		auto const parentLabelID = resolveParentLabelID(id);
+
+		auto const originalLabelIndex = m_labels.idToLabelIndex(parentLabelID);
+		auto const& originalLabel = originalLabels[originalLabelIndex];


This is a string view, not a string. Is there any point in taking a reference to it?

Suggested change

auto const& originalLabel = originalLabels[originalLabelIndex];

std::string_view const originalLabel = originalLabels[originalLabelIndex];

BTW, this yet another good example of how auto obfuscates things that would have been obvious otherwise.

it is a string :) case in point, though!

cameel · 2025-03-07T21:58:56Z

libyul/optimiser/LabelIDDispenser.cpp

+		auto const& parentLabel = originalLabels[parentLabelIndex];
+
+		std::string generatedLabel = parentLabel;
+		size_t suffix = 1;


One small optimization we could do here would be to remember the counter value we stopped at when we last visited the same parent.

Not sure if it's worth it in that form, because it would require us to keep track of counters for all labels from the original registry, but a simplified version would be to just use a single counter for all labels. That could be achieved simply by moving the suffix variable initialization out of the loop. The downside being that numbers used in names would have more digits than they could otherwise.

Good idea! The memory impact isn't too big, just a bunch of size_ts.

cameel · 2025-03-07T22:04:46Z

libyul/optimiser/LabelIDDispenser.h

+	/// A set of reserved labels may be provided, which is excluded when generating new labels. If a reserved label
+	/// already appears in the label registry and is used as-is in the AST, it will be reused despite it being
+	/// provided here.
+	explicit LabelIDDispenser(


One important thing to mention is that the original labels will be preserved even if they're not valid Yul identifiers.

And for generateNewLabels() I'd add that labels are guaranteed to be valid and not reserved IFF the original registry satisfies this condition. And it's guaranteed that at least no additional invalid/reserved names will be introduced.

Makes sense, noted these things.

cameel · 2025-03-07T22:10:56Z

libyul/optimiser/LabelIDDispenser.cpp

+	if (usedIDs.empty())
+		return {};


This condition will never be satisfied, because ASTLabelRegistry always contains at least one label (the empty one) and therefore usedIDs will always contain 0.

Another consequence of including 0 in usedIDs is that you'll try to generate a new label for it (you also mark it as reused), which will overwrite idToLabelMap[0]. I think that'll actually trip an assertion in ASTLabelRegistry constructor, because it requires that the label at 0 is "".

On the other hand the "" you put in labels will go unreferenced (unless there are unused IDs in the original registry).

This condition will never be satisfied, because ASTLabelRegistry always contains at least one label (the empty one) and therefore usedIDs will always contain 0.

Right. Although this is a bit temporary and to be replaced with whatever the NameCollector yields on an input block in which it may happen to have an empty usedIDs set. I have removed the condition for now so it can be re-introduced when properly integrating the block input.

cameel · 2025-03-07T22:18:21Z

libyul/optimiser/LabelIDDispenser.cpp

+	m_reservedLabels(_reservedLabels.begin(), _reservedLabels.end()),
+	m_offset(_labels.maxID() + 1)
+{
+	m_reservedLabels += std::set{""};


Is it even necessary to reserve ""? As far as I can tell, m_reservedLabels just prevents a label from being reused in generateNewLabels(), but you set reusedLabels[0] = true there, so it won't hit the reserved name check anyway.

Good catch. I don't think it is necessary and having fewer reserved labels is good for performance. :)

libyul/optimiser/LabelIDDispenser.cpp

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

clonker commented Feb 7, 2025

View reviewed changes

clonker requested a review from blishko February 7, 2025 11:12

blishko reviewed Feb 7, 2025

View reviewed changes

libyul/optimiser/NodeIdDispenser.h Outdated Show resolved Hide resolved

libyul/optimiser/NodeIdDispenser.h Outdated Show resolved Hide resolved

clonker force-pushed the add_node_id_dispenser branch 2 times, most recently from 10dde18 to 0076c71 Compare February 10, 2025 07:45

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

clonker force-pushed the add_node_id_dispenser branch from 0076c71 to 26ea8ed Compare February 10, 2025 16:42

cameel reviewed Feb 11, 2025

View reviewed changes

libyul/optimiser/OptimizerUtilities.h Outdated Show resolved Hide resolved

clonker force-pushed the add_yul_name_label_registry branch 4 times, most recently from 0fdc32e to 5bc5d98 Compare February 13, 2025 10:53

clonker force-pushed the add_node_id_dispenser branch from 26ea8ed to 96f8138 Compare February 13, 2025 11:11

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

clonker force-pushed the add_node_id_dispenser branch from 96f8138 to 1ae159b Compare February 13, 2025 11:26

clonker force-pushed the add_yul_name_label_registry branch 2 times, most recently from a6ae0c7 to b0f5c6b Compare February 14, 2025 13:11

clonker force-pushed the add_node_id_dispenser branch from 1ae159b to 37112b5 Compare February 14, 2025 13:47

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

clonker force-pushed the add_node_id_dispenser branch from 37112b5 to ada52c0 Compare February 14, 2025 14:30

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

clonker force-pushed the add_node_id_dispenser branch 2 times, most recently from b514841 to 9f54785 Compare February 20, 2025 08:14

cameel force-pushed the add_yul_name_label_registry branch from f330e45 to 4d96648 Compare March 1, 2025 00:11

Base automatically changed from add_yul_name_label_registry to develop March 1, 2025 05:29

clonker force-pushed the add_node_id_dispenser branch from 9f54785 to f50d498 Compare March 4, 2025 09:12

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

cameel reviewed Mar 7, 2025

View reviewed changes

libyul/optimiser/LabelIDDispenser.cpp Outdated Show resolved Hide resolved

clonker force-pushed the add_node_id_dispenser branch 4 times, most recently from 0d84ba2 to d2d9d5b Compare March 11, 2025 15:08

clonker mentioned this pull request Mar 14, 2025

Name dependent CSE bug: Compare ASM only #15942

Merged

Yul: add label id dispenser

d7aa648

clonker force-pushed the add_node_id_dispenser branch from d2d9d5b to d7aa648 Compare March 25, 2025 10:43

clonker mentioned this pull request Mar 26, 2025

Integrate numerical label IDs to Yul #15969

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add id dispenser for numerical yul node ids #15838

Add id dispenser for numerical yul node ids #15838

clonker commented Feb 7, 2025 •

edited

Loading

clonker Feb 7, 2025

cameel Mar 7, 2025

clonker Mar 10, 2025

cameel Mar 7, 2025

cameel Mar 7, 2025

clonker Mar 10, 2025

cameel Mar 7, 2025

clonker Mar 10, 2025

cameel Mar 7, 2025

clonker Mar 10, 2025

cameel Mar 7, 2025

clonker Mar 10, 2025

cameel Mar 7, 2025

clonker Mar 10, 2025

cameel Mar 7, 2025

clonker Mar 10, 2025

cameel Mar 7, 2025

clonker Mar 10, 2025

cameel Mar 7, 2025

cameel Mar 7, 2025

clonker Mar 10, 2025

cameel Mar 7, 2025

clonker Mar 10, 2025

		// this can be replaced by the actually used ids in the provided block once the AST uses ids instead of YulString
		std::set<NodeId> usedIds = ranges::views::iota(static_cast<size_t>(0), m_mapping.size() + m_offset) \| ranges::to<std::set<NodeId>>;

	std::set<NodeId> usedIds = NameCollector(_root).names();
	// add ghosts to usedIds as they're not referenced in the regular ast
	for (size_t i = 0; i < m_mapping.size(); ++i)
	if (m_mapping[i] == ASTNodeRegistry::ghostId())
	usedIds.insert(i + m_offset);

		}


		ASTLabelRegistry LabelIDDispenser::generateNewLabels(Block const&, Dialect const& _dialect) const


		#include <libyul/optimiser/LabelIDDispenser.h>

		#include <libyul/optimiser/NameCollector.h>

	auto const& originalLabel = originalLabels[originalLabelIndex];
	std::string_view const originalLabel = originalLabels[originalLabelIndex];

Add id dispenser for numerical yul node ids #15838

Are you sure you want to change the base?

Add id dispenser for numerical yul node ids #15838

Conversation

clonker commented Feb 7, 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

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

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

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 commented Feb 7, 2025 •

edited

Loading