transformations: linalg.generic to loops factor out some helpers #2662
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2662 +/- ##
=======================================
Coverage 89.68% 89.69%
=======================================
Files 364 364
Lines 46901 46920 +19
Branches 7114 7119 +5
=======================================
+ Hits 42065 42084 +19
Misses 3733 3733
Partials 1103 1103 ☔ View full report in Codecov by Sentry. |
superlopuh
force-pushed
the
sasha/memref_stream/loop-utils-outline
branch
from
fd59df9 to
24fd1a9
Compare
superlopuh
force-pushed
the
sasha/memref_stream/loop-utils-location
branch
from
7a73b0d to
32c68d2
Compare
| ], | ||
| ) -> Sequence[SSAValue]: | ||
| """ | ||
| Creates a perfect loop nest, populating the innermost body with the provided |
There was a problem hiding this comment.
SHould the title hint that we care about a perfect lops nest?
There was a problem hiding this comment.
Sorry, I mean the function name
There was a problem hiding this comment.
Ah, maybe, although since it's a private function, I think it's sort of safe to drop the implicit perfection
| rewriter.insert_op_at_location(loop, insertion_point) | ||
| results = loop.results | ||
|
|
||
| if i + 1 == len(bounds): |
There was a problem hiding this comment.
What is this line trying to check?
There was a problem hiding this comment.
that it's the last iteration in the loop
There was a problem hiding this comment.
ok, sorry I thought by default enumerate starts at 1 for a moemnt.
…ref_stream/loop-utils-outline
| def _insert_load_ops( | ||
| rewriter: PatternRewriter, | ||
| insertion_point: InsertPoint, | ||
| ind_vars: Sequence[BlockArgument], | ||
| affine_map_attrs: Sequence[AffineMapAttr], | ||
| operands: Sequence[SSAValue], | ||
| args: Sequence[BlockArgument], | ||
| load: Callable[ | ||
| [SSAValue, Sequence[SSAValue], PatternRewriter, InsertPoint], SSAValue | ||
| ], | ||
| ) -> Sequence[tuple[int, SSAValue]]: |
There was a problem hiding this comment.
Would love a docstring on this chonker
| def _insert_store_ops( | ||
| rewriter: PatternRewriter, | ||
| insertion_point: InsertPoint, | ||
| ind_vars: Sequence[BlockArgument], | ||
| output_indexing_maps: Sequence[AffineMapAttr], | ||
| yield_operands: Sequence[SSAValue], | ||
| output_operands: Sequence[SSAValue], | ||
| store: Callable[[SSAValue, SSAValue, Sequence[SSAValue]], Operation], | ||
| ): |
There was a problem hiding this comment.
Same here, seeing this many operands without a docstring makes me nervous
There was a problem hiding this comment.
Why does it need all these things to insert the stores (e.g. what are the yield_operands?)
…oop-utils-outline
|
I'm happy to make changes! Although it would be easier with more specific feedback. Like what isn't clear, etc. |
|
I'll merge this for now, but am more than happy to open a new PR to add the further clarifications you recommend |
In order to lower to an imperfectly-nested loop structure, it is useful to have a factored-out set of helpers, introduced in this PR.
Note stacked PR.