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 90.19%. Comparing base (3d4d51d) to head (b9d927b).
Report is 16 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3650      +/-   ##
==========================================
+ Coverage   90.17%   90.19%   +0.02%     
==========================================
  Files         458      458              
  Lines       58304    58530     +226     
  Branches     5694     5722      +28     
==========================================
+ Hits        52573    52794     +221     
  Misses       4339     4339              
- Partials     1392     1397       +5

☔ 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

superlopuh · 2025-02-22T12:08:05Z

Just opened #3947, which fixes the above issue. Would you have the time to iterate on this in the coming weeks?

watermelonwolverine · 2025-02-24T08:32:22Z

Hi, I am very busy with my thesis, but I think I can still fit this in somehow. I'll update this PR once the other PR is merged. Should I still split this PR into smaller PRs?

superlopuh · 2025-02-24T09:21:36Z

That would be fantastic, but also please let me know if there's another time to check in for this, there's no particular rush, it's just nice to be able to get to 0 PRs eventually.

…Expr

Remove compose_with_values function as it is in essence the same as eval

…ead_transfer_write_ops

…r_read_transfer_write_ops

…nsfer_write_ops

… AffineExpr (#4007) Add a couple of utility functions to AffineMap and AffineExpr that deal with dimensions. Cherry picked from #3650, prerequisite for implementing `vector.transfer_read` and `vector.transfer_write` ops

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

compor reviewed Dec 18, 2024

View reviewed changes

compor added 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 …

Loading
Loading status checks…

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…

Loading
Loading status checks…

b2abf0a

…ead_transfer_write_ops

Fixed formatting

Loading
Loading status checks…

a78a39b

watermelonwolverine added 3 commits December 19, 2024 15:22

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

Loading
Loading status checks…

7874eb6

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

Loading
Loading status checks…

47c55e9

…f transfer ops from MLIR

Fixed formatting

Loading
Loading status checks…

f2c6f15

watermelonwolverine added 2 commits December 28, 2024 17:21

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

Loading
Loading status checks…

5b6c700

Transferred more filecheck tests for transfer ops from MLIR

Fixed vector.tranfer_* filechecks

Loading
Loading status checks…

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…

Loading
Loading status checks…

b331962

…nd depend on issue xdslproject#3654 to be fixed

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

Loading
Loading status checks…

aee9476

…ead_transfer_write_ops

watermelonwolverine added 2 commits March 3, 2025 10:26

Add AffineMap.compose_with_values and AffineExpr.as_constant

Loading
Loading status checks…

3610be9

Add functions dealing with dimensions to AffineMap and AffineExpr

Loading
Loading status checks…

c5972fb

This was referenced Mar 3, 2025

core: (affine) Add Affine.binary #4006

Open

core: (affine) Add functions dealing with dimensions to AffineMap and AffineExpr #4007

Merged

watermelonwolverine added 8 commits March 4, 2025 09:35

Add AffineExpr.binary op that mimics AffineExpr mlir::getAffineSymbol…

Loading
Loading status checks…

e559dcd

…Expr

Refactoring in affine_expr.py

Loading
Loading status checks…

01518ad

Implement suggested changes

Loading
Loading status checks…

b2c57a8

Remove compose_with_values function as it is in essence the same as eval

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

75a9b88

…ead_transfer_write_ops

Merge branch 'affine_add_compose_with_values' into vector_add_transfe…

b0062ab

…r_read_transfer_write_ops

Merge branch 'affine_add_dim_funcs' into vector_add_transfer_read_tra…

a4041eb

…nsfer_write_ops

TransferOps: Re-added verication of inferred mask type

18a6d2e

Fix issues with vector.transfer ops

Loading
Loading status checks…

b9d927b

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

superlopuh commented Feb 22, 2025

watermelonwolverine commented Feb 24, 2025

superlopuh commented Feb 24, 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

superlopuh commented Feb 22, 2025

watermelonwolverine commented Feb 24, 2025

superlopuh commented Feb 24, 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