[Operations] Replace top-level join and blocker with synchronizer #557
Conversation
murphe67
requested review from
AyaElAkhras,
DanaKossaybati,
Jiahui17 and
shundroid
and removed request for
Jiahui17
|
This pull request is not fully finished (needs custom type verification so that inputs match outputs, and that bitwidths are properly extracted in rtl.cpp) but represents a simple solution to the oddness of the blocker operands. @Jiahui17 @shundroid @AyaElAkhras |
| std::vector<ProducerData> outsData; | ||
| }; | ||
|
|
||
| // Never used but let it be |
There was a problem hiding this comment.
if this is needed for synchronizer is easy to add, but this is a separate thing- we never added blocker to this
| // If the operand has many dependencies, then each of them is singularly | ||
| // connected with an SSA network, and then everything is joined. | ||
| // connected with an SSA network, and then everything is synchronized. | ||
| ValueRange operands = dependencies; | ||
| rewriter.setInsertionPointToStart(operand->getOwner()->getBlock()); | ||
| auto joinOp = rewriter.create<handshake::JoinOp>( | ||
| auto synchronizerOp = rewriter.create<handshake::SynchronizerOp>( | ||
| operand->getOwner()->getLoc(), operands); | ||
| joinOp->moveBefore(operandOwner); | ||
| synchronizerOp->moveBefore(operandOwner); | ||
|
|
||
| for (unsigned i = 0; i < dependencies.size(); i++) { | ||
| if (failed(connect(&joinOp->getOpOperand(i), dependencies[i]))) | ||
| if (failed(connect(&synchronizerOp->getOpOperand(i), dependencies[i]))) | ||
| return failure(); | ||
| } | ||
|
|
||
| operand->set(joinOp.getResult()); | ||
| operand->set(synchronizerOp.getOuts()[0]); |
There was a problem hiding this comment.
synchronizer can easily replace join, you just take the first output, second output is sinked
| if (lhsResult.getType().isa<handshake::ControlType>()) { | ||
| ValueRange joinInputs = {lhsEndBufferOp.getResult(), | ||
| ValueRange synchronizerInputs = {lhsEndBufferOp.getResult(), | ||
| rhsEndBufferOp.getResult()}; | ||
| JoinOp joinOp = | ||
| builder.create<JoinOp>(builder.getUnknownLoc(), joinInputs); | ||
| setHandshakeAttributes(builder, joinOp, BB_OUT, eqName); | ||
| miterResultValues.push_back(joinOp.getResult()); | ||
| SynchronizerOp synchronizerOp = | ||
| builder.create<SynchronizerOp>(builder.getUnknownLoc(), synchronizerInputs); | ||
| setHandshakeAttributes(builder, synchronizerOp, BB_OUT, eqName); | ||
| miterResultValues.push_back(synchronizerOp.getOuts()[0]); |
There was a problem hiding this comment.
join replacement here
There was a problem hiding this comment.
will add comment of example output
There was a problem hiding this comment.
need to add custom naming to this so that it has operands of ins0, ins1, ins2 instead of ins_0, ins_1, ins_2.
when the underscore is present the netlist generator expects it to be a 2d signal.
sharing wrapper also uses this trick to have a variable number of operands but 1d port assignments.
ultimately we should make this be set in a sensible matter, but we are stuck with what we have for now
There was a problem hiding this comment.
need to add custom type verification to this to make sure each output has the bitwidth of its corresponding input
| let arguments = (ins Variadic<ChannelType>:$data); | ||
| let results = (outs ChannelType:$result); | ||
| let assemblyFormat = "$data attr-dict `:` type($result)"; | ||
| let assemblyFormat = "$ins `:` type($ins) attr-dict `->` type($outs)"; |
There was a problem hiding this comment.
not sure if this is the best format, we don't have any other super varying input types like this, but apparently this is a common format?
need to research this a little bit more
| .Case<handshake::SynchronizerOp>([&](auto) { | ||
| // Number of input channels | ||
| addUnsigned("SIZE", op->getNumOperands()); | ||
| }) |
There was a problem hiding this comment.
blocker op has a bug here since it doesnt register datatype which will break phase 2 matching @AyaElAkhras , will add this later also
| }) | ||
| .Case<handshake::BufferOp, handshake::ForkOp, handshake::LazyForkOp, | ||
| handshake::BranchOp, handshake::SinkOp>([&](auto) { | ||
| handshake::BranchOp, handshake::SinkOp, handshake::SynchronizerOp>([&](auto) { |
There was a problem hiding this comment.
@Carmine50 this operation does not have "a" bitwidth, it will have many- just returning the first bitwidth for now, not sure what our options for supporting this in-depth are
| let builders = [ | ||
| OpBuilder<(ins "ValueRange":$ins), [{ | ||
| SmallVector<Type> outTypes; | ||
| for (Value v : ins) | ||
| outTypes.push_back(v.getType()); | ||
| build($_builder, $_state, outTypes, ins); | ||
| }]> | ||
| ]; |
There was a problem hiding this comment.
@shundroid custom builder to infer out types from in types
|
@Jiahui17 probably I make another PR after this to make the new python sharing wrapper call this instead of pulling from sharing wrapper support? |
|
ElasticMiter is still using the old dot2smv on the main branch. #391 updates it to use the new SMV backend. |
|
That runs from Handshake right? If we temporarily add the join back in to the tablegen and fabric generation.cpp, but leave it out of the backend/hardware stuff, this would be mergeable? |
2 participants
The top-level join and blocker operations represent an awkward design:
This pull request adds the synchronizer operation, which represents the functionality of both units. Tokens of any types can be synchronized together: their values are unchanged by this, but their valid signals are passed through a join and eager fork so that tokens may only exit the unit when all inputs have arrived. The tokens do not have to leave the synchronizer together, as it is an eager fork.
This is a strong change, as removing the old join operation forces the fabric generation for elastic miter into the beta backend earlier than the rest of Dynamatic.