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

dialects: (builtin) change data representation of DenseIntOrFPElements to use bytes #3623

Merged
merged 1 commit into from
Jan 8, 2025
Merged

dialects: (builtin) change data representation of DenseIntOrFPElements to use bytes #3623

merged 1 commit into from
Jan 8, 2025

Conversation

jorendumoulin
Copy link
Collaborator

@jorendumoulin jorendumoulin commented Dec 11, 2024 •
edited
Loading

No description provided.

@jorendumoulin jorendumoulin self-assigned this Dec 11, 2024
@jorendumoulin jorendumoulin added the dialects Changes on the dialects label Dec 11, 2024
Base automatically changed from joren/print-float-precision to main December 12, 2024 08:46
@jorendumoulin jorendumoulin force-pushed the joren/dense-attr-bytes branch 3 times, most recently from 62a6866 to 8a721ad Compare December 12, 2024 11:57
@jorendumoulin jorendumoulin marked this pull request as ready for review December 12, 2024 11:59
@codecov Codecov
Copy link

codecov bot commented Dec 12, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.31%. Comparing base (73834d3) to head (603b0f1).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3623      +/-   ##
==========================================
- Coverage   91.31%   91.31%   -0.01%     
==========================================
  Files         468      468              
  Lines       58571    58578       +7     
  Branches     5652     5655       +3     
==========================================
+ Hits        53485    53491       +6     
- Misses       3634     3635       +1     
  Partials     1452     1452

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

alexarice
alexarice reviewed Dec 12, 2024
View reviewed changes
@jorendumoulin
Copy link
Collaborator Author

Oh, I was completely unaware of the zero-rank tensor, sorry! I wrongfully assumed some of the tests were just a bit quirky.

What still bothers me is the following test:

xdsl/tests/dialects/test_builtin.py

Line 220 in 8a721ad

check1 = DenseIntOrFPElementsAttr.tensor_from_list([4, 5], f32, [])

this shouldn't be legal, right? storing two values in a zero-rank tensor?

Does the following make sense as expected behaviour?

    # legal zero-rank tensor
    attr = DenseIntOrFPElementsAttr.tensor_from_list([4], f32, [])
    assert attr.type == AnyTensorType(f32, [])

    # illegal zero-rank tensor
    with pytest.raises(ValueError, match="A zero-rank tensor can only hold 1 value but 2 were given."):
        DenseIntOrFPElementsAttr.tensor_from_list([4, 5], f32, [])

    # legal normal tensor
    attr = DenseIntOrFPElementsAttr.tensor_from_list([4, 5], f32, [2])
    assert attr.type == AnyTensorType(f32, [2])

superlopuh
superlopuh reviewed Dec 13, 2024
View reviewed changes
xdsl/dialects/builtin.py Outdated
Comment on lines 2125 to 2139
if isinstance(self.type.element_type, AnyFloat):
return tuple(
FloatAttr(value, self.type.element_type)
for value in cast(tuple[float, ...], self.get_values())
)
elif isinstance(self.type.element_type, IntegerType):
return tuple(
IntegerAttr(value, self.type.element_type)
for value in cast(tuple[int, ...], self.get_values())
)
else: # index
return tuple(
IntegerAttr(value, self.type.element_type)
for value in cast(tuple[int, ...], self.get_values())
)
Copy link
Member

Choose a reason for hiding this comment

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

This would be a great helper in a separate PR, I think, and also provided for DenseArrayBase. I wonder if we should change iter/unpack to iter_values/unpack_values and iter_attrs/unpack_attrs

Copy link
Member

Choose a reason for hiding this comment

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

#3706

@superlopuh superlopuh mentioned this pull request Jan 6, 2025
Merged
superlopuh added a commit that referenced this pull request Jan 6, 2025
@superlopuh
dialects: (builtin) add unpack and iter_unpack to IntegerAttr and Flo…
061f484 
…atAttr (#3706)

This is useful for `DenseArrayBase`, and will be useful for
`DenseIntOrFPElementsAttr` once that migrates to be backed by bytes.
#3623
@superlopuh superlopuh mentioned this pull request Jan 7, 2025
Open
@superlopuh
Copy link
Member

Just for the sake of ideal PRs, it would be great to split out all the changes that aren't in builtin.py to a separate PR, so that the underlying storage change is transparent, if that's not too tedious!

@jorendumoulin
Copy link
Collaborator Author

Just for the sake of ideal PRs, it would be great to split out all the changes that aren't in builtin.py to a separate PR, so that the underlying storage change is transparent, if that's not too tedious!

No problem! See #3715 , #3716, #3717, #3718

@jorendumoulin jorendumoulin force-pushed the joren/dense-attr-bytes branch from e8d321f to 603b0f1 Compare January 7, 2025 17:05
@jorendumoulin
Copy link
Collaborator Author

This is now rebased, only remaining with changes in the class definition itself!

superlopuh
superlopuh approved these changes Jan 7, 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.

🚀🚀

@jorendumoulin jorendumoulin merged commit 78bea80 into main Jan 8, 2025
16 checks passed
@jorendumoulin jorendumoulin deleted the joren/dense-attr-bytes branch January 8, 2025 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexarice alexarice alexarice left review comments

@superlopuh superlopuh superlopuh approved these changes

@compor compor compor approved these changes

@math-fehr math-fehr Awaiting requested review from math-fehr

Assignees

@jorendumoulin jorendumoulin

Labels
dialects Changes on the dialects
Projects
None yet
Milestone
No milestone
Development

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