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: Add AffineMap #1029

Merged
merged 8 commits into from Jun 6, 2023
Merged

core: Add AffineMap #1029

merged 8 commits into from Jun 6, 2023

Conversation

Groverkss
Copy link
Collaborator

@Groverkss Groverkss commented May 30, 2023

This patch adds AffineMap to core ir. AffineMap allows modeling many dialects in upstream MLIR like Linalg, Affine, Memref (load/store), SCF, etc.

@Groverkss Groverkss changed the title WIP: Add AffineExpr class to core ir WIP: Add AffineExpr to core ir May 30, 2023
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Patch coverage: 80.10% and project coverage change: -0.32 ⚠️

Comparison is base (d86bab8) 87.00% compared to head (e0bf077) 86.69%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1029      +/-   ##
==========================================
- Coverage   87.00%   86.69%   -0.32%     
==========================================
  Files         125      132       +7     
  Lines       19197    20656    +1459     
  Branches     2914     3129     +215     
==========================================
+ Hits        16702    17907    +1205     
- Misses       1999     2235     +236     
- Partials      496      514      +18     
Impacted Files Coverage Δ
xdsl/affine_ir.py 74.65% <74.65%> (ø)
tests/test_affine_builtins.py 100.00% <100.00%> (ø)

... and 69 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some style comments! Otherwise, the code seems good!
I would just like more documentation, and probably some more tests


# (5 * d0) + s0 + 1
c = (a * 5) + b + 1
print(c)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For testing here, I would assert that this is equal to AffineExpr constructed manually (without the operators).

xdsl/ir.py Outdated Show resolved Hide resolved
xdsl/ir.py Outdated Show resolved Hide resolved
xdsl/ir.py Outdated
SymbolId = ("s",)


class _AffineExprStorage(ABC):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you write a bit of documentation for each of these classes? Mostly to help us understand.

xdsl/ir.py Outdated Show resolved Hide resolved
xdsl/ir.py Outdated Show resolved Hide resolved
xdsl/ir.py Outdated Show resolved Hide resolved
xdsl/ir.py Outdated Show resolved Hide resolved
xdsl/ir.py Outdated Show resolved Hide resolved
xdsl/ir.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@webmiche webmiche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! This stil needs some more tests and some cleaning, but it still is exciting to get this into xDSL. The first step towards xDFPL 👍

xdsl/ir.py Outdated Show resolved Hide resolved
tests/test_affine_builtins.py Outdated Show resolved Hide resolved
@AntonLydike
Copy link
Collaborator

AntonLydike commented May 31, 2023

Super happy to see affine being added, thanks @Groverkss! You're doing the lords work!

@tobiasgrosser
Copy link
Contributor

tobiasgrosser commented Jun 4, 2023

@Groverkss, this is a great addition. I am looking forward to seeing this upstreamed. This will really allow us to parse and use one of the most unconventional MLIR dialects, with this funny AffineExpr encoding. Cool! I would love to see this merged.

xdsl/ir.py Outdated
Comment on lines 1619 to 1626
Add = ("+",)
Mul = ("*",)
Mod = ("mod",)
FloorDiv = ("floordiv",)
CeilDiv = ("ceildiv",)
Constant = ("const",)
DimId = ("d",)
SymbolId = ("s",)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can certainly be done better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

xdsl/ir.py Outdated Show resolved Hide resolved
xdsl/ir.py Outdated Show resolved Hide resolved
xdsl/ir.py Outdated Show resolved Hide resolved
@Groverkss Groverkss changed the title WIP: Add AffineExpr to core ir core: Add AffineMap Jun 5, 2023
xdsl/ir.py Outdated Show resolved Hide resolved
xdsl/ir.py Outdated Show resolved Hide resolved
xdsl/ir.py Outdated Show resolved Hide resolved
xdsl/ir.py Outdated Show resolved Hide resolved
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.

very cool

Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

xdsl/affine_ir.py Show resolved Hide resolved
xdsl/affine_ir.py Show resolved Hide resolved
@math-fehr math-fehr merged commit a77fc33 into xdslproject:main Jun 6, 2023
11 checks passed
cigarichard pushed a commit to cigarichard/xdsl that referenced this pull request Jun 17, 2023
This patch adds AffineMap to core ir. AffineMap allows modeling many
dialects in upstream MLIR like Linalg, Affine, Memref (load/store), SCF,
etc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants