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.

Sign up for GitHub

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

Jump to bottom

parser: parse bytes directly into a BytesAttr for DenseIntOrFPElementsAttrs #3845

Merged
merged 3 commits into from
Feb 7, 2025
Merged

parser: parse bytes directly into a BytesAttr for DenseIntOrFPElementsAttrs #3845

merged 3 commits into from
Feb 7, 2025

Conversation

jorendumoulin
Copy link
Collaborator

Because DenseIntOrFPElementsAttrs are backed by bytes, there is no need to convert the hex representation to a list of values when creating the attribute. We can directly convert the hex representation to a BytesAttr when constructing the attribute.

This is a lot faster, and gets rid of a lot of code somewhat duplicating the StructPackable things we made.

@jorendumoulin jorendumoulin changed the title parser: do not parse bytes for DenseIntOrFPElementsAttrs parser: parse bytes directly into a BytesAttr for DenseIntOrFPElementsAttrs Feb 5, 2025
@jorendumoulin jorendumoulin self-assigned this Feb 5, 2025
@jorendumoulin jorendumoulin marked this pull request as draft February 5, 2025 13:57
@superlopuh
Copy link
Member

Nice! Just for fun, could you please add a benchmark here for the before/after?
https://github.com/xdslproject/xdsl-bench

@jorendumoulin
Copy link
Collaborator Author

Nice! Just for fun, could you please add a benchmark here for the before/after? https://github.com/xdslproject/xdsl-bench

I will have a look!

@codecov Codecov
Copy link

codecov bot commented Feb 5, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.41%. Comparing base (8ee6ed9) to head (498b942).
Report is 8 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3845      +/-   ##
==========================================
+ Coverage   91.26%   91.41%   +0.14%     
==========================================
  Files         461      461              
  Lines       57656    58940    +1284     
  Branches     5570     5849     +279     
==========================================
+ Hits        52619    53879    +1260     
+ Misses       3612     3606       -6     
- Partials     1425     1455      +30

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jorendumoulin jorendumoulin marked this pull request as ready for review February 5, 2025 15:13
superlopuh
superlopuh approved these changes Feb 5, 2025
View reviewed changes
Copy link
Member

@superlopuh superlopuh left a comment

Choose a reason for hiding this comment

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

Changes look good! Let's merge this tomorrow, after we have a "before" run on https://xdsl.dev/xdsl-bench/

@jorendumoulin jorendumoulin reopened this Feb 5, 2025
@jorendumoulin
Copy link
Collaborator Author

(I misclicked)

@jorendumoulin
Copy link
Collaborator Author

Changes look good! Let's merge this tomorrow, after we have a "before" run on https://xdsl.dev/xdsl-bench/

Sounds good!
Could it be that the CI is not running anymore since Feb 1 over there?

@jorendumoulin jorendumoulin mentioned this pull request Feb 5, 2025
Merged
@superlopuh
Copy link
Member

Oh, weird, indeed

superlopuh pushed a commit that referenced this pull request Feb 5, 2025
@jorendumoulin
dialects: (builtin) print hex str for DenseIntOrFPElementsAttrs with …
2b2f51e 
…> 100 elements (#3846)

mimicking mlir behaviour

This, along with #3845 should allow for very fast printing/parsing of
large dense attrs, allowing for low-overhead calling of intermediate
mlir-opt passes in our pipeline.
emmau678 pushed a commit that referenced this pull request Feb 6, 2025
@jorendumoulin @emmau678
dialects: (builtin) print hex str for DenseIntOrFPElementsAttrs with …
9f50033 
…> 100 elements (#3846)

mimicking mlir behaviour

This, along with #3845 should allow for very fast printing/parsing of
large dense attrs, allowing for low-overhead calling of intermediate
mlir-opt passes in our pipeline.
@compor
Copy link
Collaborator

compor commented Feb 7, 2025

The benchmarks are in (e.g., https://xdsl.dev/xdsl-bench/#parser.time_parser__dense_attr_hex)

We are good to merge, I think

@superlopuh
Copy link
Member

Nice!

@superlopuh superlopuh merged commit 04a50fc into main Feb 7, 2025
31 checks passed
@superlopuh superlopuh deleted the joren/parse-bytes branch February 7, 2025 10:45
@jorendumoulin
Copy link
Collaborator Author

How exciting, I can't wait to see the numbers tomorrow

oluwatimilehin pushed a commit to oluwatimilehin/xdsl that referenced this pull request Feb 13, 2025
@jorendumoulin @oluwatimilehin
dialects: (builtin) print hex str for DenseIntOrFPElementsAttrs with …
fec98c1 
…> 100 elements (xdslproject#3846)

mimicking mlir behaviour

This, along with xdslproject#3845 should allow for very fast printing/parsing of
large dense attrs, allowing for low-overhead calling of intermediate
mlir-opt passes in our pipeline.
oluwatimilehin pushed a commit to oluwatimilehin/xdsl that referenced this pull request Feb 13, 2025
@jorendumoulin @oluwatimilehin
parser: parse bytes directly into a BytesAttr for DenseIntOrFPElement…
40f004a 
…sAttrs (xdslproject#3845)

Because DenseIntOrFPElementsAttrs are backed by `bytes`, there is no
need to convert the hex representation to a list of values when creating
the attribute. We can directly convert the hex representation to a
BytesAttr when constructing the attribute.

This is a lot faster, and gets rid of a lot of code somewhat duplicating
the `StructPackable` things we made.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@superlopuh superlopuh superlopuh approved these changes

@compor compor compor approved these changes

@alexarice alexarice Awaiting requested review from alexarice

Assignees

@jorendumoulin jorendumoulin

Labels
parser
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jorendumoulin @superlopuh @compor