core: Implement constant folding for AffineExpr #1178
Conversation
Groverkss
requested review from
math-fehr,
webmiche and
georgebisbas
as code owners
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1178 +/- ##
==========================================
+ Coverage 88.72% 88.75% +0.03%
==========================================
Files 161 161
Lines 22398 22455 +57
Branches 3385 3409 +24
==========================================
+ Hits 19873 19931 +58
+ Misses 1983 1981 -2
- Partials 542 543 +1
☔ View full report in Codecov by Sentry. |
|
|
||
| # Keep the constant on the right side. | ||
| if self._impl.kind == _AffineExprKind.Constant: | ||
| self, other = other, self |
There was a problem hiding this comment.
That's so cursed ;) (But that's fine)
There was a problem hiding this comment.
Any better way to swap things?
There was a problem hiding this comment.
No that's the best!
The only curse thing in my opinion is swapping self, but that's fine!
There was a problem hiding this comment.
Maybe something like other.__add__(self)?
| @@ -159,9 +159,42 @@ def eval(self, dims: list[int], symbols: list[int]) -> int: | |||
|
|
|||
| raise ValueError("Unreachable") | |||
|
|
|||
| def _try_fold_constant( | |||
There was a problem hiding this comment.
Before you merge this, why not make the signature of this function
| def _try_fold_constant( | |
| def _try_fold_constant(self) -> AffineExpr: |
Where you return self if you could not merge the constant? Where the implementation folds the constant in the lhs, rhs, and then matches on the kind of the operation to see how they could be combined?
There was a problem hiding this comment.
Also it seems like swapping around the order of arguments might be confusing, feels like that's a separate thing from constant folding to me.
There was a problem hiding this comment.
Also it seems like swapping around the order of arguments might be confusing, feels like that's a separate thing from constant folding to me.
It's canonicalizing the input for simplifications. Seems pretty standard to me?
There was a problem hiding this comment.
Before you merge this, why not make the signature of this function
Where you return self if you could not merge the constant? Where the implementation folds the constant in the lhs, rhs, and then matches on the kind of the operation to see how they could be combined?
I like the current implementation. It allows me to fold things like (expr + constant) + constant too.
This will be done as more simplifications are added. This patch just adds some of them. |
|
I added some more simplifications for now, but I don't think I'll add any more simplifications for now. |
Groverkss
force-pushed
the
affineexprconstantfold
branch
from
2a6ebf3
to
be58dff
Compare
No description provided.