-
Notifications
You must be signed in to change notification settings - Fork 73
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: (stream) remove stream dialect and move types to memref_stream and snitch #3510
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3510 +/- ##
=======================================
Coverage 90.32% 90.33%
=======================================
Files 464 463 -1
Lines 58067 58095 +28
Branches 5561 5552 -9
=======================================
+ Hits 52451 52481 +30
+ Misses 4187 4186 -1
+ Partials 1429 1428 -1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
@@ -593,7 +593,7 @@ func.func public @pooling_nchw_max_d1_s2_3x3( | |||
#snitch_stream.stride_pattern<ub = [128], strides = [8]> | |||
] | |||
} ins(%X_1 : !riscv.reg) outs(%Y_1 : !riscv.reg) { | |||
^0(%x : !stream.readable<!riscv.freg<ft0>>, %0 : !stream.writable<!riscv.freg<ft1>>): | |||
^0(%x : !snitch.readable<!riscv.freg<ft0>>, %0 : !snitch.writable<!riscv.freg<ft1>>): |
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.
What was the decision process for when to use snitch
and when to use memref_stream
? This file for example uses both.
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.
wdym? this file was used to go from snitch/risc-v all the way up to linalg when modelling the lowering for the other way around.
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 is probably just me not knowing much about this side of xDSL, but in this PR some cases of !stream.(readable|writable)
are changed to !memref_stream....
and some are changed to !snitch....
and I couldn't really see a pattern. As a result I feel I can't really review this at the moment (though I wasn't pinged for a review (maybe for this reason)).
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.
it's entirely determined by the operation enclosing the block in question. In this case it's a snitch_stream.streaming region, in others it's memref_stream.streaming region.
One fewer stream dialects in the MLIR cinematic universe, makes it easier to do things like restraint snitch streams to always have floating point registers in the future.