core: Support hex denseAttr parsing for integers, f32, f64 #1974

JosseVanDelm · 2024-01-12T16:28:26Z

Attempt at solving #1971

This was a bit more tricky than I initially expected due to endianess issues.

Some things I noticed going back and forth between xdsl-opt and mlir-opt:

mlir-opt has the tendency to print signless values as signed, whereas xdsl seems to print them as unsigned.
here i'm adding the integer values by their signed value, is that okay?
mlir-opt prints floats slightly different from xdsl, i made sure the values are same-ish manually

Currently still draft:

improve negative tests
improve shape testing/verifying?
Can probably be made a bit prettier, suggetions welcome!

I'm leaving other float types for future work, these seem not possible to be unpacked with python's struct package.

codecov · 2024-01-12T16:31:43Z

Codecov Report

Attention: 25 lines in your changes are missing coverage. Please review.

Comparison is base (78e6d14) 89.83% compared to head (f3296a9) 89.95%.
Report is 4 commits behind head on main.

Files	Patch %	Lines
xdsl/pattern_rewriter.py	68.29%	9 Missing and 4 partials ⚠️
xdsl/parser/attribute_parser.py	90.76%	3 Missing and 3 partials ⚠️
xdsl/backend/riscv/register_allocation.py	90.32%	1 Missing and 2 partials ⚠️
.../riscv/lowering/convert_snitch_stream_to_snitch.py	89.47%	1 Missing and 1 partial ⚠️
xdsl/dialects/riscv_snitch.py	97.77%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1974      +/-   ##
==========================================
+ Coverage   89.83%   89.95%   +0.12%     
==========================================
  Files         284      284              
  Lines       35531    35649     +118     
  Branches     5241     5259      +18     
==========================================
+ Hits        31918    32069     +151     
+ Misses       2840     2822      -18     
+ Partials      773      758      -15

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

xdsl/parser/attribute_parser.py

JosseVanDelm · 2024-01-15T14:08:40Z

xdsl/parser/attribute_parser.py

+        # Splat attribute case, same value everywhere
+        if chunk_size == len(byte_list) and type.get_shape() != (1,):
+            return data_values, []


@math-fehr @superlopuh @compor this might not be a sufficient check here. Any suggestions?

if len(data_values) == 1 should be a check that is enough?
Any 1x... dense attribute is a splat anyway

superlopuh · 2024-01-15T14:50:20Z

xdsl/parser/attribute_parser.py

+
+        if isa(element_type, Float32Type | Float64Type):
+            for i in range(num_chunks):
+                parsed_float = struct.unpack(


Should we use unpack_from or iter_unpack? feels like we could avoid the iteration in Python, and instead check that the number was what we expected

Changed to using unpack_from, thanks for the suggestion!

superlopuh · 2024-01-15T14:52:04Z

xdsl/parser/attribute_parser.py

+                values, shape = [], None
+            else:
+                # Expect a tensor literal instead
+                values, shape = self._parse_tensor_literal()


Why not put the str literal logic inside the _parse_tensor_literal code? Do you know how MLIR structures this parsing logic?

I haven't looked at the parser code of MLIR tbh, I just parsed some denseAttrs in compiler explorer and took a look at what was produced. The MLIR codebase scares me a bit haha, but it seems that they put it inside the TensorLiteralParser?
https://github.com/llvm/llvm-project/blob/d85df3f2d6e8687c44e6802dcc0e59c14ff32c9b/mlir/lib/AsmParser/AttributeParser.cpp#L456

It should not be in _parse_tensor_literal, as the hex string can only be at the toplevel in MLIR

compor · 2024-01-15T14:47:36Z

xdsl/parser/attribute_parser.py

+            hex_string = self.parse_optional_str_literal()
+            if hex_string is not None:
+                # Can not determine values without type yet
+                values, shape = [], None
+            else:
+                # Expect a tensor literal instead
+                values, shape = self._parse_tensor_literal()


I have trouble understanding what hex_string, values, shape are expected to be at each line here.

Can this be simplified for the case where values, shape = [], None?

Can this be combined with the _parse_builtin_dense_attr_hex below?
You seem to assume this is a hex, but you don't really know that yet, wouldn't determining this as fast as possible make sense and potentially elide the extra check if hex_string is not None: below?

Ideally, I'd like to come out of the block knowing that if hex exists is valid.

Hi @compor I agree, that this is a bit of an ugly way of going about this.

I think it looks ugly because of the following problems I had:

The problem is that I can not really extract the shape information out of the hex_string since the information is already flattened inside the hex attribute, so there are a few checks you can not perform on hex values.

The added code kind of breaks the previous flow because you already need your type info to be able to parse the hex string in the first place. In the previous code this was not a problem, because you can already parse the ints and the floats purely on the format of the numbers

another problem is that I was afraid of emitting _TensorLiteralElements because they seem to require a span, and I was unsure on how this works. @superlopuh would this solve your question as well?

Does this make sense? Thanks for your help!

To answer your actual question @compor:

Originally:
shape = [] for splat attribute, list of ints for bigger attributes, e.g. [[2,3],[3,2]] would give a shape = [2,2]
values = a list of _TensorLiteralElements which contains parsed TensorLiteral elements

Now:
hex_string The value of the hex string, if present, None if not present
shape = [] for splat attribute, list of ints for bigger attributes, e.g. [[2,3],[3,2]] would give a shape = [2,2] .
For hex attributes I can only give the flattened shape as a single-element list (e.g. shape = [4]) because of a lack of information.
values = a list of _TensorLiteralElements which contains parsed TensorLiteral elements
In the case of a hex attribute, the values are already in their python counterparts, so values is already of type list[int] or list[float] and doesn't need to be converted from TensorLiteralElement anymore.

I agree that this is a lot of info necessary for just 3 variables 😅 , any suggestions very much welcome!

I agree that what you're saying makes sense, our parser API might be a little awkward for this, hopefully we can make it a little less awkward to deal with it better. I'd say that the span can be the span of the string literal for all the elements for your last bullet.

@math-fehr, what are your thoughts on all this?

I had a similar issue with this BTW, when I tried to implement memref.global custom syntax: https://xdsl.zulipchat.com/#narrow/stream/368604-Framework/topic/printing.20and.20parsing.20attributes/near/410844828

We could just put the entire span for the string, so we don't have list[int] or list[float] and have the list of literal elements, I would be fine with that!
We won't use these errors anyway, so there is no problems.

To simplify the logic actually, can we just split the following code in two functions? One function to deal with the hex, and the other to deal with the lists?
Because currently the logic gets quite complex

Thanks @math-fehr I'll try to refactor to using the literals now and I'll see what I'm left with 😄

On second thought @math-fehr it doesn't make a lot of sense to make the _TensorLiteralElements, because then you have to convert them to a list of ints going out of the function anyways :(

xdsl/parser/attribute_parser.py

JosseVanDelm · 2024-01-15T16:36:03Z

Okay it seems there's some further work necessary beyond your comments.
I got this error on a 2D shape:

Shape mismatch in dense literal. Expected [640, 128] shape from the type, but got [81920] shape.

math-fehr

Looks like it's going to the right direction, the only problem is the code getting complex now

Co-authored-by: Chris Vasiladiotis <cvassiladiotis@gmail.com>

superlopuh · 2024-01-16T14:53:30Z

xdsl/parser/attribute_parser.py

+        # Get values and shape in case of hex_string (requires parsed type)
+        if hex_string is not None:
+            values, shape = self._parse_builtin_dense_attr_hex(hex_string, type)


I might be missing something, but what changes if this line is inlined above, and the hex_string doesn't escape the else: branch above? I have a feeling that this method would be easier to understand with an is_hex_string bool value instead of the string, just to communicate that it's not used for anything once its parsed

superlopuh · 2024-01-16T14:55:07Z

xdsl/parser/attribute_parser.py

        # Convert list of elements to a list of values.
-        if shape != []:
-            data_values = [value.to_type(self, element_type) for value in values]
+        # For hex string, the data values are already constructed


Is this true? Isn't one of your examples a 2xi32 array where there's a single repeated value decoded?

Co-authored-by: Chris Vasiladiotis <cvassiladiotis@gmail.com>

superlopuh · 2024-01-17T14:27:52Z

Definitely looks much neater, thank you!

superlopuh · 2024-01-17T14:28:23Z

I'd still recommend inlining the helpers that are only called once in your new version

superlopuh · 2024-01-17T14:32:50Z

Here's what I came to after a bit of fiddling, it's not exactly what I recommended before, but also seems like a direction to look into: sasha/add-hex-parser

JosseVanDelm · 2024-01-17T17:09:39Z

@superlopuh I like your patch! Can you add it to this PR?

superlopuh · 2024-01-17T17:50:31Z

pushed! @math-fehr, can you please review my latest patch?

superlopuh · 2024-01-18T14:11:48Z

Got the ok from Mathieu, thank you very much @JosseVanDelm !

JosseVanDelm added enhancement New feature or request core xDSL core (ir, textual format, ...) labels Jan 12, 2024

JosseVanDelm self-assigned this Jan 12, 2024

superlopuh reviewed Jan 12, 2024

View reviewed changes

xdsl/parser/attribute_parser.py Outdated Show resolved Hide resolved

compor self-requested a review January 15, 2024 10:51

JosseVanDelm changed the title ~~[WIP] core: Support hex denseAttr parsing for integers, f32, f64~~ core: Support hex denseAttr parsing for integers, f32, f64 Jan 15, 2024

JosseVanDelm marked this pull request as ready for review January 15, 2024 14:07

JosseVanDelm requested review from superlopuh and math-fehr January 15, 2024 14:07

JosseVanDelm commented Jan 15, 2024

View reviewed changes

JosseVanDelm force-pushed the Josse/add-hex-parser branch from 85f5a54 to de15ff3 Compare January 15, 2024 14:38

superlopuh reviewed Jan 15, 2024

View reviewed changes

compor reviewed Jan 15, 2024

View reviewed changes

math-fehr approved these changes Jan 16, 2024

View reviewed changes

JosseVanDelm and others added 9 commits January 16, 2024 15:37

Add hex parser for integers

a83f12c

Add tests for endianess

92b1de8

Add support for f32, f64

c0d55e2

Add error messages and negative tests

f7894e5

Refactor to helper function

224f693

Unpack with list comprehension

ffcfd9f

Use unpack from instead of python iterator

6f336ac

Remove typo nits

fbf1ed8

Co-authored-by: Chris Vasiladiotis <cvassiladiotis@gmail.com>

Perform flattened shape test for hex attributes

9f85c08

JosseVanDelm force-pushed the Josse/add-hex-parser branch from e2377ad to 9f85c08 Compare January 16, 2024 14:43

superlopuh reviewed Jan 16, 2024

View reviewed changes

Change check for splat attributes

590b700

JosseVanDelm and others added 18 commits January 17, 2024 15:23

Add tests for endianess

c3085e9

Add support for f32, f64

e24ae34

Add error messages and negative tests

cf0b3f6

Refactor to helper function

1cef9a7

Unpack with list comprehension

38705b1

Use unpack from instead of python iterator

e581cba

Remove typo nits

7287d75

Co-authored-by: Chris Vasiladiotis <cvassiladiotis@gmail.com>

Perform flattened shape test for hex attributes

8a95344

Change check for splat attributes

0326954

Refactor _parse_dense_type

898f42e

Refactor checks of dense attribute parser

3691257

More refactoring

ecea2a2

Refactor even more

cd4e212

Pass arguments to check functions

689f3fb

Remove stray comment

0d7e9b1

refactor: Merge if statements

6a8aab8

refactor: Improve code reuse on parse_dense_type

0efbb8e

Fix bug in tensor literal order

6c13c64

JosseVanDelm force-pushed the Josse/add-hex-parser branch from 2276424 to 6c13c64 Compare January 17, 2024 14:23

Merge branch 'Josse/add-hex-parser' into sasha/add-hex-parser

f3296a9

superlopuh merged commit 6348624 into main Jan 18, 2024
10 checks passed

superlopuh deleted the Josse/add-hex-parser branch January 18, 2024 14:11

JosseVanDelm mentioned this pull request Jan 18, 2024

xDSL can not parse a DenseAttr with Hex values #1971

Closed

JosseVanDelm mentioned this pull request Jul 11, 2024

Wrong values on large dense arrays after xdsl-opt #2876

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Support hex denseAttr parsing for integers, f32, f64 #1974

core: Support hex denseAttr parsing for integers, f32, f64 #1974

JosseVanDelm commented Jan 12, 2024 •

edited

Loading

codecov bot commented Jan 12, 2024 •

edited

Loading

JosseVanDelm Jan 15, 2024

math-fehr Jan 16, 2024

superlopuh Jan 15, 2024

JosseVanDelm Jan 15, 2024

superlopuh Jan 15, 2024

JosseVanDelm Jan 15, 2024

math-fehr Jan 16, 2024

compor Jan 15, 2024 •

edited

Loading

JosseVanDelm Jan 15, 2024

JosseVanDelm Jan 15, 2024

superlopuh Jan 15, 2024

superlopuh Jan 16, 2024

math-fehr Jan 16, 2024

math-fehr Jan 16, 2024

JosseVanDelm Jan 16, 2024

JosseVanDelm Jan 16, 2024

JosseVanDelm commented Jan 15, 2024

math-fehr left a comment

superlopuh Jan 16, 2024

superlopuh Jan 16, 2024

superlopuh commented Jan 17, 2024

superlopuh commented Jan 17, 2024

superlopuh commented Jan 17, 2024

JosseVanDelm commented Jan 17, 2024

superlopuh commented Jan 17, 2024

superlopuh commented Jan 18, 2024

core: Support hex denseAttr parsing for integers, f32, f64 #1974

core: Support hex denseAttr parsing for integers, f32, f64 #1974

Conversation

JosseVanDelm commented Jan 12, 2024 • edited Loading

codecov bot commented Jan 12, 2024 • edited Loading

Codecov Report

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

compor Jan 15, 2024 • 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

JosseVanDelm commented Jan 15, 2024

math-fehr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superlopuh commented Jan 17, 2024

superlopuh commented Jan 17, 2024

superlopuh commented Jan 17, 2024

JosseVanDelm commented Jan 17, 2024

superlopuh commented Jan 17, 2024

superlopuh commented Jan 18, 2024

JosseVanDelm commented Jan 12, 2024 •

edited

Loading

codecov bot commented Jan 12, 2024 •

edited

Loading

compor Jan 15, 2024 •

edited

Loading