dialects: (vector) Add for `vector.transfer_read` and `vector.transfer_write` operations #3650

watermelonwolverine · 2024-12-17T23:19:49Z

Added stubs for "vector.transfer_read" and "vector.transfer_write".
Verification is missing but that also seems to be a lot of work.

Also fixed a small bug in TensorOrMemrefOf.verify (?)

Fixed bug in TensorOrMemrefOf.verify (?)

codecov · 2024-12-17T23:26:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.26%. Comparing base (668176f) to head (aee9476).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3650      +/-   ##
==========================================
- Coverage   91.27%   91.26%   -0.01%     
==========================================
  Files         462      462              
  Lines       57680    57864     +184     
  Branches     5565     5590      +25     
==========================================
+ Hits        52645    52810     +165     
- Misses       3613     3626      +13     
- Partials     1422     1428       +6

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

compor

LGTM modulo comments

compor · 2024-12-18T08:47:33Z

xdsl/dialects/builtin.py

-        if isinstance(attr, VectorType) or isinstance(attr, TensorType):
-            attr = cast(VectorType[Attribute] | TensorType[Attribute], attr)
+        if isinstance(attr, MemRefType) or isinstance(attr, TensorType):
+            attr = cast(MemRefType[Attribute] | TensorType[Attribute], attr)


Would you mind moving this to a separate PR with a test case?
We seem to be lacking any verification testing on this type.

Seeing the changes, I'd still recommend doing this.

I will do that in the next few days

xdsl/dialects/vector.py

….transfer_write" ops This blew up and should now be several PRs

…or.transfer_write" ops

…ArrayAttr[BoolAttr] instead of a IntegerAttr

watermelonwolverine · 2024-12-18T18:18:46Z

I started implementing the verification and finished most of it
- Only verification for cases where the element_type of source is a vector type is missing
Problem: To fully implement verification as it is done in MLIR the VectorType needs to be changed first
- It needs a bitmap (ArrayAttr[BoolAttr]) for the scalable dimensions instead of a single IntegerAttr for the number of scalable dimensions.
- I could skip this verification step and do everything else
- I did some of those changes so you can see for yourself what I mean.
I also had to add several functions to builtin classes like AffineMap and AffineExpr.
- Should I do a separate PR for those, too?

vector.transfer_*: Disabled mask and inferred_mask comparison for now until VectorType is fixed

…r.py

…ead_transfer_write_ops

watermelonwolverine · 2024-12-19T11:58:24Z

Reverted changes done to VectorType.
Added unit tests for function added to AffineExpr and AffineMap
Fixed some bugs with verification and did some refactoring
Disabled code parts that rely on issue dialects: (builtin) VectorType should have a ArrayAttr[BoolAttr] for scalable_dims instead of an IntAttr for num_scalable_dims #3654 to be fixed

…f transfer ops from MLIR

compor · 2024-12-20T15:46:09Z

You can always verify what is currently here and document what deviates (as you have done), this is fine IMHO.

Thanks for the work so far!

Transferred more filecheck tests for transfer ops from MLIR

Added the (now mandatory) in_bounds property

superlopuh · 2024-12-31T17:44:43Z

xdsl/dialects/vector.py

+    # WJOG9GVF: TODO fix: uncomment this when 7S4F0FZA has been fixed
+    # if mask_type:
+    #     if mask_type != inferred_mask_type:
+    #         raise VerifyException(
+    #             f'"{op.name}" inferred mask type ({inferred_mask_type}) and mask operand type ({mask_type}) don\'t match'
+    #         )


Same as below. I will move it to a new pull request

xdsl/dialects/vector.py

…ead_transfer_write_ops

…nd depend on issue xdslproject#3654 to be fixed

superlopuh · 2025-02-07T08:10:54Z

Hi @watermelonwolverine! I'd love to get this in, please let me know if you'd like some help with the PR. From my understanding, these bits of code that are currently in this PR would benefit from their own mini PRs:

is_function_of_dim + test
compose_with_values + test
used/unused_dims + test

Do you have time to have a go at these? I'm hoping it will be a fairly easy cherry-picking process, I'm happy to review each one very quickly :) After that, I think this PR should be ready to merge with minor changes, but it would be good to reduce it first.

…ead_transfer_write_ops

watermelonwolverine · 2025-02-07T13:05:51Z

This PR is actually pretty much done from my side.
There are a few verifications I had to remove because they depend on #3654 being fixed. I plan to create a new pull request afterwards to add the verifications back in.
There are a few TODOs in verification and testing, but but I think those deserve to be their own PRs.

watermelonwolverine · 2025-02-07T13:08:29Z

I can split this PR into smaller PRs over the next few days if it is simply too big

superlopuh · 2025-02-07T14:16:35Z

Yeah that would be great, thank you

dialects: (vector) Added stubs for transfer_read and transfer_write

383fc93

Fixed bug in TensorOrMemrefOf.verify (?)

watermelonwolverine marked this pull request as draft December 17, 2024 23:23

compor reviewed Dec 18, 2024

View reviewed changes

compor added the dialects Changes on the dialects label Dec 18, 2024

compor assigned watermelonwolverine Dec 18, 2024

compor changed the title ~~dialects: (vector) Added stubs for transfer_read and transfer_write~~ dialects: (vector) Add for vector.transfer_read and vector.transfer_write operations Dec 18, 2024

watermelonwolverine added 3 commits December 18, 2024 17:26

Started working on verification of "vector.transfer_read" and "vector…

85a7c83

….transfer_write" ops This blew up and should now be several PRs

Continued working on verification of "vector.transfer_read" and "vect…

c8ec10f

…or.transfer_write" ops

Did some follow-up changes that resulted from VectorType expecting a …

ff8b9f9

…ArrayAttr[BoolAttr] instead of a IntegerAttr

watermelonwolverine added 7 commits December 19, 2024 10:40

Reverted changes done to VectorType.

9c87737

vector.transfer_*: Disabled mask and inferred_mask comparison for now until VectorType is fixed

Fixed bugs for vector.transfer_*

77d8acb

Did some refactoring in affine_map and affine_expr

b7f554c

Added tests for newly added functions in affine_map.py and affine_exp…

0013c20

…r.py

Added unused_dims_bit_vector function to AffineMap

9217d31

Merge remote-tracking branch 'origin/main' into vector_add_transfer_r…

b2abf0a

…ead_transfer_write_ops

Fixed formatting

a78a39b

watermelonwolverine added 3 commits December 19, 2024 15:22

Transferred a bunch of lit tests for vector.transfer_* ops from MLIR

7874eb6

Fixed bug in TransferWriteOp.__init__ and transfered more lit tests o…

47c55e9

…f transfer ops from MLIR

Fixed formatting

f2c6f15

watermelonwolverine added 2 commits December 28, 2024 17:21

Fixed a few issues with vector.transfer_read and vector.transfer_write

5b6c700

Transferred more filecheck tests for transfer ops from MLIR

Fixed vector.tranfer_* filechecks

a702250

Added the (now mandatory) in_bounds property

watermelonwolverine marked this pull request as ready for review December 28, 2024 17:48

superlopuh reviewed Dec 31, 2024

View reviewed changes

xdsl/dialects/vector.py Outdated Show resolved Hide resolved

superlopuh reviewed Dec 31, 2024

View reviewed changes

xdsl/dialects/vector.py Outdated Show resolved Hide resolved

superlopuh reviewed Dec 31, 2024

View reviewed changes

xdsl/dialects/vector.py Outdated Show resolved Hide resolved

watermelonwolverine added 2 commits January 2, 2025 14:17

Merge remote-tracking branch 'origin/main' into vector_add_transfer_r…

0ff1569

…ead_transfer_write_ops

Perfomed some clean-up and undid all changes which aren't ready yet a…

b331962

…nd depend on issue xdslproject#3654 to be fixed

Merge remote-tracking branch 'origin/main' into vector_add_transfer_r…

aee9476

…ead_transfer_write_ops

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: (vector) Add for `vector.transfer_read` and `vector.transfer_write` operations #3650

dialects: (vector) Add for `vector.transfer_read` and `vector.transfer_write` operations #3650

watermelonwolverine commented Dec 17, 2024

codecov bot commented Dec 17, 2024 •

edited

Loading

compor left a comment

compor Dec 18, 2024

compor Dec 20, 2024

watermelonwolverine Dec 23, 2024

watermelonwolverine commented Dec 18, 2024 •

edited

Loading

watermelonwolverine commented Dec 19, 2024

compor commented Dec 20, 2024

superlopuh Dec 31, 2024

watermelonwolverine Dec 31, 2024

superlopuh commented Feb 7, 2025 •

edited

Loading

watermelonwolverine commented Feb 7, 2025

watermelonwolverine commented Feb 7, 2025

superlopuh commented Feb 7, 2025

dialects: (vector) Add for vector.transfer_read and vector.transfer_write operations #3650

Are you sure you want to change the base?

dialects: (vector) Add for vector.transfer_read and vector.transfer_write operations #3650

Conversation

watermelonwolverine commented Dec 17, 2024

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

compor left a comment

Choose a reason for hiding this comment

compor Dec 18, 2024

Choose a reason for hiding this comment

compor Dec 20, 2024

Choose a reason for hiding this comment

watermelonwolverine Dec 23, 2024

Choose a reason for hiding this comment

watermelonwolverine commented Dec 18, 2024 • edited Loading

watermelonwolverine commented Dec 19, 2024

compor commented Dec 20, 2024

superlopuh Dec 31, 2024

Choose a reason for hiding this comment

watermelonwolverine Dec 31, 2024

Choose a reason for hiding this comment

superlopuh commented Feb 7, 2025 • edited Loading

watermelonwolverine commented Feb 7, 2025

watermelonwolverine commented Feb 7, 2025

superlopuh commented Feb 7, 2025

dialects: (vector) Add for `vector.transfer_read` and `vector.transfer_write` operations #3650

dialects: (vector) Add for `vector.transfer_read` and `vector.transfer_write` operations #3650

codecov bot commented Dec 17, 2024 •

edited

Loading

watermelonwolverine commented Dec 18, 2024 •

edited

Loading

superlopuh commented Feb 7, 2025 •

edited

Loading