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

[WIP] TokenPartitionTree refactoring / "Choice" nodes support #1328

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

mglb
Copy link
Contributor

@mglb mglb commented Apr 30, 2022

Objectives

  • Support multiple node types (subclasses) in TokenPartitionTree instead of one super-node
  • Implement Choice node which directly maps to Choice operator in Layout Optimizer

Design decisions

TokenPartitionTree uses a variant for storing nodes instead of regular polymorphism

Main reason: I wanted to do something that would work without requiring bigger changes in code that uses TokenPartitionTree. Regular polymorphism would change node objects stored in Children list into pointers, which would require a lot of changes at once in the whole codebase.
With the use of a variant there was only one thing that required a change in the whole code base: Children() now returns pointer instead of reference, and can return nullptr. This is mostly change of . into ->. nullptr is handled mostly through the use of is_leaf() instead of Children()->empty().

Minor reason: this will allow linear storage of child nodes in memory. I didn't do any benchmarks, so I can only guess whether this helps much.

Custom Variant implementation

I've implemented Variant class from scratch instead of using std::variant.

Main reason: I wanted a possibility to "cast" pointer to object stored in variant into a pointer to the Variant holding it. This requires a guarantee that the variant's data member which stores the value is the first member, or at least access to this member in order to use offsetof. Neither is provided by std::variant.

This possibility is used in TokenPartition node implementations (which are stored in a Variant) to "cast" this into pointer to Variant, which is then assigned as parent pointer in child nodes.

@mglb mglb force-pushed the mglb/TokenPartitionTreeRefactor branch from 06ae10c to 7d77d45 Compare April 30, 2022 16:25
@codecov-commenter
Copy link

codecov-commenter commented Apr 30, 2022

Codecov Report

Merging #1328 (818c6b4) into master (b534c1f) will decrease coverage by 0.18%.
The diff coverage is 87.26%.

@@            Coverage Diff             @@
##           master    #1328      +/-   ##
==========================================
- Coverage   92.87%   92.69%   -0.19%     
==========================================
  Files         340      341       +1     
  Lines       23693    24048     +355     
==========================================
+ Hits        22006    22292     +286     
- Misses       1687     1756      +69     
Impacted Files Coverage Δ
common/formatting/tree_unwrapper.h 91.66% <ø> (ø)
common/util/container_proxy.h 100.00% <ø> (ø)
common/util/logging.h 100.00% <ø> (ø)
common/util/value_saver.h 100.00% <ø> (ø)
common/util/vector_tree.h 98.66% <ø> (ø)
verilog/formatting/formatter.cc 93.62% <ø> (ø)
common/formatting/token_partition_tree.cc 87.69% <60.58%> (-10.64%) ⬇️
common/formatting/tree_unwrapper.cc 76.64% <62.50%> (+3.72%) ⬆️
common/formatting/align.cc 89.31% <85.71%> (ø)
common/formatting/token_partition_tree.h 86.30% <86.30%> (-13.70%) ⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b534c1f...818c6b4. Read the comment docs.

@mglb mglb force-pushed the mglb/TokenPartitionTreeRefactor branch from 7d77d45 to 73947e5 Compare April 30, 2022 17:15
@mglb mglb force-pushed the mglb/TokenPartitionTreeRefactor branch 4 times, most recently from 818c6b4 to 352d73a Compare May 10, 2022 16:51
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