[DCToHW] Add DCToHW conversion pass #5298
Conversation
mortbopet
force-pushed
the
dev/mpetersen/dc_to_hw
branch
from
1e89f53
to
f5588fc
Compare
| hw.module @select(%sel : !dc.value<i1>, %true : !dc.token, %false : !dc.token) -> (out0: !dc.token) { | ||
| %0 = dc.select %sel, %true, %false | ||
| hw.output %0 : !dc.token | ||
| } |
There was a problem hiding this comment.
Since I'm only reviewing for style, I'm only commenting.
This code over-uses auto: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable. Personally, I try to only use auto to (1) unbox, (2) auto op = b.create<comb::Addop>, and (3) iterators.
Some of my comments in RTLBuilder are geared towards moving it into a support lib.
lib/Conversion/DCToHW/DCToHW.cpp
Outdated
| Type outType = | ||
| llvm::TypeSwitch<Type, Type>(t) | ||
| .Case<ValueType>([&](auto vt) { | ||
| return esi::ChannelType::get(ctx, toHWType(vt.getInnerTypes())); |
There was a problem hiding this comment.
By default, ESI channels have ValidReady signaling. Sufficient for a first take, but very quickly we'll need to move to something more complex. May even be selecting different signaling standards based on site-specific criteria (so a pass option might not work). No changes suggested for now -- just something to bear in mind.
lib/Conversion/DCToHW/DCToHW.cpp
Outdated
| InputHandshake hs; | ||
| auto ready = std::make_shared<Backedge>(bb.get(rtlb.b.getI1Type())); | ||
| auto [data, valid] = rtlb.unwrap(in, *ready); | ||
| hs.valid = valid; |
There was a problem hiding this comment.
nit: You're constructing a few structs like this. Visually, I prefer using a struct initializer with /*ready=*/ comments. Would allow you to condense 6 lines to one (using emplace_back).
FFS C++: WHY DID YOU NOT GET STRUCT DESIGNATED INITIALIZERS UNTIL '20?! You're only 21 years behind C, which has had this feature since C99!
| /// Locate the clock and reset values from the parent operation based on | ||
| /// attributes assigned to the arguments. | ||
| static FailureOr<std::pair<Value, Value>> getClockAndReset(Operation *op) { | ||
| auto *parent = op->getParentOp(); |
There was a problem hiding this comment.
Could this be getParentOpOfType<FuncOpIface>? Or do you specifically want to restrict dc.clock to be in the same block args?
There was a problem hiding this comment.
I feel either approach is somewhat vague. Choosing the parent function-like op was just the closest thing to "give me the parent op which has attributes attached to its arguments". I'm inclined to leave this as-is for now, until we know that this is not a scaleable approach.
This commit adds a lowering of DC operation to a combination of
`comb,seq` and `hw` operations. The implementations are identical to
those of the `handshake` dialect (i.e. `handshake-to-hw` sans the
arithmetic operations).
- ESI-typed I/O
- DC buffers lower to ESI buffers
- This is just a temporary solution. I anticipate more complexity here
in the future when we want to do more with the FIFOs that connect
DC circuits.
- In case the IR contains DC operations that need to be clocked (fork, buffer),
there must exist a clock and reset signal in the parent `FunctionLike`
operation. These arguments are to be marked with a `dc.clock` and `dc.reset`
attribute, respectively.
- Only a single integration test for now. Will add more once #5297 lands
(allows us to do some high-level tests through handshake).
mortbopet
force-pushed
the
dev/mpetersen/dc_to_hw
branch
from
9563ffa
to
315366d
Compare
This commit adds a lowering of DC operation to a combination of
comb,seqandhwoperations. The implementations are identical to those of thehandshakedialect (i.e.handshake-to-hwsans the arithmetic operations).FunctionLikeoperation. These arguments are to be marked with adc.clockanddc.resetattribute, respectively.--map-arith-to-combpass #5297 lands (allows us to do some high-level tests through handshake).