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: (affine) Add Affine.binary #4006

Merged

Conversation

watermelonwolverine
Copy link
Contributor

@watermelonwolverine watermelonwolverine commented Mar 3, 2025

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

Copy link

codecov bot commented Mar 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (e209b0c) to head (06a5eba).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4006      +/-   ##
==========================================
- Coverage   90.23%   90.23%   -0.01%     
==========================================
  Files         458      458              
  Lines       58409    58455      +46     
  Branches     5691     5697       +6     
==========================================
+ Hits        52707    52747      +40     
- Misses       4320     4323       +3     
- Partials     1382     1385       +3     

☔ 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.

@watermelonwolverine
Copy link
Contributor Author

While implementing this I realized that when composing two affine maps the expressions are not automatically simplified/constant folded.
So, a resulting AffineBinaryOpExpr where both lhs and rhs are AffineConstantExpr is not simplified to a AffineConstantExpr.
That's why I added the AffineExpr.as_constant function. Is this by design or should the constant folding happen while composing?

@superlopuh
Copy link
Member

What does MLIR do here? I have a suspicion that they constant fold when creating the affine expr, it would be worth doing the same in xDSL. I can't find these helpers in the AffineExpr and AffineMap header files.

@watermelonwolverine watermelonwolverine changed the title Add AffineMap.compose_with_values and AffineExpr.as_constant core: Add AffineMap.compose_with_values and AffineExpr.as_constant Mar 4, 2025
@watermelonwolverine
Copy link
Contributor Author

MLIR simplifies while composing. This is the call stack, the simplification happens at the last call:

AffineExpr AffineExpr::compose(AffineMap map)
AffineExpr AffineExpr::replaceDimsAndSymbols(ArrayRef<AffineExpr> dimReplacements,ArrayRef<AffineExpr> symReplacements)
AffineExpr mlir::getAffineBinaryOpExpr(AffineExprKind kind, AffineExpr lhs, AffineExpr rhs)
AffineExpr AffineExpr::operator+(AffineExpr other)
static AffineExpr simplifyAdd(AffineExpr lhs, AffineExpr rhs)

@watermelonwolverine
Copy link
Contributor Author

Added AffineExpr.binary which mimics AffineExpr mlir::getAffineBinaryOpExpr from AffineExpr.cpp.
Now it should be pretty close to how MLIR is doing it

@watermelonwolverine watermelonwolverine changed the title core: Add AffineMap.compose_with_values and AffineExpr.as_constant core: Add AffineMap.compose_with_values and AffineExpr.binary Mar 4, 2025
Remove compose_with_values function as it is in essence the same as eval
@watermelonwolverine watermelonwolverine changed the title core: Add AffineMap.compose_with_values and AffineExpr.binary core: Add Affine.binary Mar 4, 2025
@compor compor changed the title core: Add Affine.binary core: (affine) Add Affine.binary Mar 5, 2025
@compor compor added the core xDSL core (ir, textual format, ...) label Mar 5, 2025
@superlopuh superlopuh requested a review from compor March 11, 2025 13:33
@superlopuh superlopuh merged commit 821862d into xdslproject:main Mar 12, 2025
17 checks passed
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.

3 participants