dialects: (transform) Fix amount of returned loops in TileOp #3080
Conversation
xdsl/dialects/transform.py
Outdated
| len(static_sizes.as_tuple()) | ||
| if isinstance(static_sizes, DenseArrayBase) | ||
| else 0 | ||
| self.amount_of_non_zero_sizes(static_sizes, dynamic_sizes) |
There was a problem hiding this comment.
Why this name? Is it from MLIR?
There was a problem hiding this comment.
I ask this because our usual convention would make this num_non_zero_sizes), but if MLIR calls it this way then we can follow their convention, as long as we add a link to their implementation.
There was a problem hiding this comment.
BTW this implementation is a little surprising, could you please document the logic?
There was a problem hiding this comment.
Hi, sorry for the delayed answer. In MLIR this isn't computed using specific helper function at all. rather they use the count-function to reach this goal.
unsigned numExpectedLoops = staticTileSizes.size() - llvm::count(staticTileSizes, 0);
As xDSL does not have any such function. I implemented this helper function myself directly. is it better to keep the logic the same as in MLIR? of coarse I can use the count function on the Array as Tuple
There was a problem hiding this comment.
That's interesting, why not replicate this logic? It seems to not use the dynamic sizes, which makes sense to me, as we can't really reason about them. The Python spelling of this function would be:
len(static_sizes) - static_sizes.count(0), which is quite different from the implementation you propose.
There was a problem hiding this comment.
i see what you're saying, fixed it!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3080 +/- ##
=======================================
Coverage 89.89% 89.89%
=======================================
Files 417 417
Lines 52948 52953 +5
Branches 8215 8215
=======================================
+ Hits 47597 47602 +5
Misses 4022 4022
Partials 1329 1329 ☔ View full report in Codecov by Sentry. |
This PR fixes a bug where, when an array contains zeros, unneccessary loops are created. the total amount of return loops should be equal to the amount of non zero values