google / google/xls

Drop Node::SetId()

Open
#1,601 0 comments 0 reactions 0 assignees View on GitHub
ir
Dominant language
C++
Stars
1.9k
Forks
283
Avg merge
2d 10h
Merged PRs (30d)
135

Description

`Node::SetId()` has a TODO to drop it. Ideally, this would be private to the parser or not even exist.

Node IDs are usually OK to treat as immutable, but they aren't while parsing. The difficulty is that nodes you haven't parsed yet might want the ID that you would otherwise use for the node you're currently parsing. You can either patch up nodes you've already parsed when you encounter a node that wants a taken ID or defer setting IDs when unspecified and go set them when you've parsed everything.

Another approach is to first scan the input for `id=` and build a set of taken IDs before parsing anything. Then when IDs are unspecified you have all the information to get the next available ID.

I did some experimentation with making ID immutable and having `FunctionBase` store nodes in a `btree_set, NodeIdLessThan>`. The idea was to make node iteration faster than the current `std::list<>`, but the benefit I saw was small (~2% speedup?).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.