-
Notifications
You must be signed in to change notification settings - Fork 299
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
[DC] Add merge lowering #6943
[DC] Add merge lowering #6943
Conversation
Modelled after the Handshake lowering, however, becomes trivial with a 2-input merge.
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 % nit comments. Again, maybe ensure that @teqdruid has time to take a look at well.
Co-authored-by: Christian Ulmann <christianulmann@gmail.com>
Co-authored-by: Christian Ulmann <christianulmann@gmail.com>
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 looks about right to me.
|
||
if (!data.empty()) { | ||
// Data-merge; mux the selected input. | ||
auto dataMux = rewriter.create<arith::SelectOp>( |
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.
Does something lower arith.select
to an hw.mux
?
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.
map-arith-to-comb
will lower this to a comb.mux
.
// Pack the data mux with the control token. | ||
mergeOutput = pack(rewriter, selectedIndexUnpacked.token, dataMux); | ||
} else { | ||
// Control-only merge; throw away the index value of the dc.merge |
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.
- Unlike
dc.merge
,handshake.merge
doesn't output the idx of the value which got selected. This is why you had alarm bells going off. - When would a
handshake.merge
have no data? Since handshake is all about dynamic data flow, I don't think it ever would. In the context of a lowering, evennone
would be considered data, yes? - If
none
is implicitly converted todc.token
, then this makes sense.
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.
If none is implicitly converted to dc.token, then this makes sense.
it is - see
if (type.isa<NoneType>())
When would a handshake.merge have no data? Since handshake is all about dynamic data flow, I don't think it ever would. In the context of a lowering, even none would be considered data, yes?
True, in practice, i've never seen this used. The DHLS lowerings used data merge
ops but these could always be boiled down to single-input merges (i.e. a no-op). So your observation is correct, and there is a good argument for removing handshake.merge
from the dialect.
Modelled after the Handshake lowering, however, becomes trivial with a 2-input merge.