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.

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

dialects: (tensor) add insertop and extractop #3480

Merged
merged 3 commits into from
Nov 20, 2024

Conversation

francescodaghero
Copy link
Contributor

This PR adds the following operations to the tensor dialect:

  • tensor.InsertOp docs
  • tensor.ExtractOp docs

that I currently use for a tree ensemble compiler implemented using xdsl (link).

I have tried to mirror the operations available in MLIR, customizing both the print/parse functions, but I am not 100% sure I did it correctly, as this is my first PR.

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.28%. Comparing base (d3647b2) to head (65364cc).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3480      +/-   ##
==========================================
+ Coverage   90.26%   90.28%   +0.02%     
==========================================
  Files         461      462       +1     
  Lines       57807    57916     +109     
  Branches     5565     5565              
==========================================
+ Hits        52180    52292     +112     
+ Misses       4190     4189       -1     
+ Partials     1437     1435       -2     

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


🚨 Try these New Features:

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.

This is great! A few comments:

  • could you please add an interop test with MLIR? We already have tests here: tests/filecheck/dialects/tensor/ops.mlir
  • could you please have a go at using assembly_format instead? There are many examples throughout the repo, and here is the MLIR doc on the syntax
  • could you please use __init__ instead of get, we've transitioned to that as a framework, should be minimal changes, also plenty of examples available

@superlopuh
Copy link
Member

I'm hoping to add bufferization to xDSL before the end of the month, interesting that you also would benefit from it! I'd love to discuss your project if you have some time, please ping me on Zulip

@francescodaghero
Copy link
Contributor Author

Thanks for your reply,

This is great! A few comments:

  • could you please add an interop test with MLIR? We already have tests here: tests/filecheck/dialects/tensor/ops.mlir

This change should already be in this commit, do I need to modify other parts of the ops.mlir?

  • could you please have a go at using assembly_format instead? There are many examples throughout the repo, and here is the MLIR doc on the syntax

I see, I did an initial try for insertOp for instance, whose assembly_format should be :

"$scalar `into` $dest `[` $indices `]` attr-dict `:` type($dest)"

(the same as in MLIR), however, this fails with the following error:

# | xdsl.utils.exceptions.PyRDLOpDefinitionError: ('Error during the parsing of the assembly format: ', (Span[63:63](text=''), "type of operand 'scalar' cannot be inferred, consider adding a 'type($scalar)' directive to the custom assembly format"))

which makes sense, but I don't know how to solve this issue for xDSL without changing the assembly format w.r.t. the MLIR one.

  • could you please use __init__ instead of get, we've transitioned to that as a framework, should be minimal changes, also plenty of examples available

Sure, no problem.

@superlopuh
Copy link
Member

Sorry, I sent the wrong path, I meant this one: tests/filecheck/mlir-conversion/with-mlir/dialects/tensor/ops.mlir we should ideally have both.

@superlopuh superlopuh added the dialects Changes on the dialects label Nov 19, 2024
@superlopuh
Copy link
Member

superlopuh commented Nov 19, 2024

I think I have a way for the assembly_format for custom syntax to work, it might be worth a try once this PR is in: #3477

@superlopuh
Copy link
Member

Code LGTM, I'd just like to try the approach enabled by the PR I linked if it is accepted in the next day or two

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.

Actually let's merge this, I'll fiddle with it on my own time

@superlopuh superlopuh merged commit 59e59ad into xdslproject:main Nov 20, 2024
15 checks passed
@superlopuh
Copy link
Member

Thank you!

EdmundGoodman pushed a commit to EdmundGoodman/xdsl that referenced this pull request Dec 6, 2024
This PR adds the following operations to the tensor dialect:
- tensor.InsertOp
[docs](https://mlir.llvm.org/docs/Dialects/TensorOps/#tensorinsert-tensorinsertop)
- tensor.ExtractOp
[docs](https://mlir.llvm.org/docs/Dialects/TensorOps/#tensorextract-tensorextractop)

that I currently use for a tree ensemble compiler implemented using xdsl
([link](https://github.com/francescodaghero/treeco/tree/main)).

I have tried to mirror the operations available in MLIR, customizing
both the print/parse functions, but I am not 100% sure I did it
correctly, as this is my first PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialects Changes on the dialects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants