-
Notifications
You must be signed in to change notification settings - Fork 77
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
dialects: (memref_stream) Add an inits field to memref_stream.generic [1/3] #2763
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2763 +/- ##
==========================================
- Coverage 89.81% 89.79% -0.02%
==========================================
Files 372 372
Lines 47817 47891 +74
Branches 7331 7353 +22
==========================================
+ Hits 42945 43004 +59
- Misses 3736 3743 +7
- Partials 1136 1144 +8 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question about unit, otherwise this PR is a bit too complex for me and my knowledge of the streaming situation to review! Just tell me if there are specific aspects you'd like my review
…ttom-up-constants
…ref_stream/const-init
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, just a small nitpick
memref_stream.generic { | ||
bounds = [#builtin.int<4>, #builtin.int<2>, #builtin.int<3>], | ||
indexing_maps = [ | ||
affine_map<(d0, d1, d2) -> (d0, d2)>, | ||
affine_map<(d0, d1, d2) -> (d2, d1)>, | ||
affine_map<(d0, d1) -> (d0, d1)> | ||
], | ||
iterator_types = ["parallel", "parallel", "reduction"] | ||
} ins(%E, %F : memref<4x2xf64>, memref<2x3xf64>) outs(%G : memref<4x3xf64>) inits(%H : f64) { | ||
^0(%e : f64, %f : f64, %acc_old : f64): | ||
%prod = arith.mulf %e, %f : f64 | ||
%acc_new = arith.addf %acc_old, %prod : f64 | ||
linalg.yield %acc_new : f64 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: These filechecks don't actually test the inits added in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This filecheck is just to check that we roundtrip correctly with printing and parsing of custom syntax, so I added a couple of ops with inits with them, there's no other functionality intended to test here
This is PR 1/3 that introduces constant initialisation to our kernel representations. This PR adds the new attribute, and adds NotImplemented to the streamification and lowering to loops, which are handled in the next two pull requests.
Note stacked PR.