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

core: (traits) Add SameOperandsAndResultType trait #3751

Merged
merged 11 commits into from
Jan 20, 2025

Conversation

compor
Copy link
Collaborator

@compor compor commented Jan 14, 2025

This PR:

  • Adds the SameOperandsAndResultType trait
  • Tests of the above

A few examples of this being tested in upstream MLIR.

@compor compor added the core xDSL core (ir, textual format, ...) label Jan 14, 2025
@compor compor self-assigned this Jan 14, 2025
@math-fehr
Copy link
Collaborator

Quick question. Looking at it, it seems that this is more complex than just the SameOperandsAndResultType in MLIR, right?
MLIR only checks that the operands and results are the same, but here you seem to check across containers? What does the check do exactly? I would probably rename the trait to not confuse it with the MLIR one. (if I'm correct to what MLIR does)

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.26%. Comparing base (dee76d4) to head (c181348).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3751      +/-   ##
==========================================
+ Coverage   91.24%   91.26%   +0.02%     
==========================================
  Files         459      461       +2     
  Lines       57298    57454     +156     
  Branches     5532     5545      +13     
==========================================
+ Hits        52279    52437     +158     
+ Misses       3593     3591       -2     
  Partials     1426     1426              

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

@compor
Copy link
Collaborator Author

compor commented Jan 14, 2025

Quick question. Looking at it, it seems that this is more complex than just the SameOperandsAndResultType in MLIR, right? MLIR only checks that the operands and results are the same, but here you seem to check across containers? What does the check do exactly? I would probably rename the trait to not confuse it with the MLIR one. (if I'm correct to what MLIR does)

Is it? I thought it also checks within the container type?
https://github.com/llvm/llvm-project/blob/8d9dcd111e5ced8135387917859dd64d67886be0/mlir/lib/IR/Operation.cpp#L1100

@math-fehr
Copy link
Collaborator

Okay my bad then, I indeed misunderstood what MLIR was doing!

@math-fehr
Copy link
Collaborator

Thanks for the pointer!

@alexarice
Copy link
Collaborator

Does this achieve anything that you can't do with range constraints? Is it just so you can write the same thing as you can in mlir?

@math-fehr
Copy link
Collaborator

My feeling is that have_compatible_shape is either impossible, or very very annoying to write in xDSL. So here, range constraints are not enough AFAIK

@alexarice
Copy link
Collaborator

I think i've misunderstood what this does then, I'll have a proper look tomorrow

@alexarice
Copy link
Collaborator

Ah, I see what this does now, I feel the name is confusing but I guess that's what mlir uses

@alexarice
Copy link
Collaborator

My feeling is that have_compatible_shape is either impossible, or very very annoying to write in xDSL. So here, range constraints are not enough AFAIK

We should figure out how to do shapes properly with constraints/irdl at some point

@compor compor merged commit 7b2c906 into main Jan 20, 2025
16 checks passed
@compor compor deleted the christos/core/traits/same-operands-and-result-types branch January 20, 2025 10:38
oluwatimilehin pushed a commit to oluwatimilehin/xdsl that referenced this pull request Feb 13, 2025
This PR:

- Adds the `SameOperandsAndResultType` trait
- Tests of the above

A
[few](https://github.com/llvm/llvm-project/blob/539b15b41a6a01017c0a555e89b7d2b62ba194d2/mlir/test/IR/traits.mlir#L41)
examples of this being tested in upstream MLIR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core xDSL core (ir, textual format, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants