[Datapath] Conversion Pass Comb to Datapath #8664
Conversation
… builder and assemblyFormat
…explicit reduction factor
…ingle compressor tree and removing zeros
There was a problem hiding this comment.
Seems like you're missing a link target in https://github.com/llvm/circt/blob/main/lib/CAPI/Conversion/CMakeLists.txt
Otherwise LGTM!
| target.addLegalDialect<datapath::DatapathDialect, comb::CombDialect, | ||
| hw::HWDialect>(); | ||
|
|
||
| target.addIllegalOp<comb::AddOp, comb::MulOp>(); |
There was a problem hiding this comment.
Isn't this redundant due to the below dynamic legality constraints?
There was a problem hiding this comment.
Thanks Martin - didn't realise that addDynamicallyLegalOp would mark the remainder as illegal - useful to know!
| ConversionPatternRewriter &rewriter) const override { | ||
| // Can only introduce compressor for three-inputs or more | ||
| if (op.getNumOperands() <= 2) | ||
| return failure(); |
There was a problem hiding this comment.
Nit: I think the preferred way to fail is to use rewriter.notifyMatchFailure which will also notify the listener (also in the pattern below).
There was a problem hiding this comment.
I think operations with numOperands() <= 2 are legal so I believe patterns won't apply in the first place?
There was a problem hiding this comment.
Removed this check as suggested
|
|
||
| // Permit 2-input adders (carry-propagate adders) | ||
| target.addDynamicallyLegalOp<comb::AddOp>( | ||
| [](comb::AddOp op) { return op.getNumOperands() == 2; }); |
There was a problem hiding this comment.
nit: Can we make unary add legal as well? They will be canonicalized anyway
| [](comb::AddOp op) { return op.getNumOperands() == 2; }); | |
| [](comb::AddOp op) { return op.getNumOperands() <= 2; }); |
| ConversionPatternRewriter &rewriter) const override { | ||
| // Can only introduce compressor for three-inputs or more | ||
| if (op.getNumOperands() <= 2) | ||
| return failure(); |
There was a problem hiding this comment.
I think operations with numOperands() <= 2 are legal so I believe patterns won't apply in the first place?
|
Thanks reviewers - suggestions implemented ready to merge once checks are completed - still awaiting commit access approval... |
Add support for lowering variadic adders and two-input multipliers to datapath operations. Now automiatng
Resulting from comb-to-datapath and canonicalize:
This will be integrated into circt-synth during a subsequent PR.