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

Attention: Patch coverage is 88.28125% with 15 lines in your changes missing coverage. Please review.

Project coverage is 89.04%. Comparing base (b85f63b) to head (ae26827).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
xdsl/dialects/vector.py	88.28%	8 Missing and 7 partials ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #3650    +/-   ##
========================================
  Coverage   89.03%   89.04%            
========================================
  Files         317      317            
  Lines       43338    43479   +141     
  Branches     5403     5436    +33     
========================================
+ Hits        38588    38714   +126     
- Misses       3407     3414     +7     
- Partials     1343     1351     +8

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Cherry picked from #3650. Prerequisite for implementing `vector.transfer_read` and `vector.transfer_write` ops

…ead_transfer_write_ops

xdsl/ir/affine/affine_expr.py

superlopuh · 2025-03-14T14:49:36Z

tests/filecheck/dialects/vector/vector_ops.mlir

@@ -1,5 +1,5 @@
 // RUN: XDSL_ROUNDTRIP
-
+#map = affine_map<(d0, d1) -> (d0)>


seems unused

Used in "permutation_map" of vector.transfer_readand vector_transfer_write line 13 and 14.

superlopuh · 2025-03-14T14:49:59Z

tests/filecheck/dialects/vector/vector_ops.mlir

@@ -10,6 +10,9 @@ builtin.module {
    "vector.maskedstore"(%0, %2, %2, %1, %6) : (memref<4x4xindex>, index, index, vector<1xi1>, vector<1xindex>) -> ()
    "vector.print"(%6) : (vector<1xindex>) -> ()
    %7 = "vector.create_mask"(%2) : (index) -> vector<2xi1>
+    %8 = "vector.transfer_read"(%0, %2, %2, %2) <{"in_bounds" = [true], "operandSegmentSizes" = array<i32: 1, 2, 1, 0>, "permutation_map" = #map}> : (memref<4x4xindex>, index, index, index) -> vector<4xindex>


would it be worth adding custom syntax here? seems quite verbose

Sure, at some point it would totally be worth to add custom syntax. But I don't have the time at the moment because of my thesis..

I'm happy to add it if you are. I think that a good way to merge this would be to first add the ops, and then add verification. I can open a new PR with your newly added operations with no verification at all, but with custom syntax, then this PR will track adding only the verifiers and verifier helpers. Does that sound OK?

Yes, sounds good

No promises on when, Ill try to do it in the next week or two! Enjoy thesis writing ;)

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

watermelonwolverine added 2 commits March 3, 2025 10:26

Add AffineMap.compose_with_values and AffineExpr.as_constant

3610be9

Add functions dealing with dimensions to AffineMap and AffineExpr

c5972fb

This was referenced Mar 3, 2025

core: (affine) Add Affine.binary #4006

Merged

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…

e559dcd

…Expr

Refactoring in affine_expr.py

01518ad

Implement suggested changes

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

b9d927b

superlopuh pushed a commit that referenced this pull request Mar 12, 2025

core: (affine) Add Affine.binary (#4006)

821862d

Cherry picked from #3650. Prerequisite for implementing `vector.transfer_read` and `vector.transfer_write` ops

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

8848cc0

…ead_transfer_write_ops

superlopuh reviewed Mar 14, 2025

View reviewed changes

xdsl/ir/affine/affine_expr.py Outdated Show resolved Hide resolved

superlopuh reviewed Mar 14, 2025

View reviewed changes

Clean-up after merge

ae26827

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

superlopuh Mar 14, 2025

watermelonwolverine Mar 14, 2025

superlopuh Mar 14, 2025

watermelonwolverine Mar 14, 2025

superlopuh Mar 14, 2025

watermelonwolverine Mar 14, 2025

superlopuh Mar 14, 2025

		@@ -1,5 +1,5 @@
		// RUN: XDSL_ROUNDTRIP

		#map = affine_map<(d0, d1) -> (d0)>

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

watermelonwolverine commented Dec 18, 2024 • edited Loading

watermelonwolverine commented Dec 19, 2024

compor commented Dec 20, 2024

Choose a reason for hiding this comment

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

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

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