Skip to content
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

[LowerComb] Truth table to mux tree lowering #5405

Merged
merged 9 commits into from
Jun 20, 2023
Merged

Conversation

teqdruid
Copy link
Contributor

@teqdruid teqdruid commented Jun 15, 2023

Add a truth_table to mux tree lowering to the LowerComb pass.

@teqdruid teqdruid added the Comb Involving the `comb` dialect label Jun 15, 2023
@sequencer
Copy link
Contributor

FYI TruthTable already has an PLA-based implementation: https://github.com/chipsalliance/chisel/blob/435d5cc43a0b49038ab4615383fc58be224d9845/src/main/scala/chisel3/util/pla.scala#L7

Do you think the truth table should conclude dont care as well?
If so, we may need to consider introducing QMC and espresso to CIRCT.

@teqdruid
Copy link
Contributor Author

@darthscsi @mikeurbach @seldridge Do you agree with adding a pass to lower this? I'll add the 'casez' lowering later.

Base automatically changed from dev/teqdruid/truthtable to main June 15, 2023 20:17
@teqdruid
Copy link
Contributor Author

Do you think the truth table should conclude dont care as well?

We specifically discussed this at a recent ODM and decided against it, at least to start.

@teqdruid teqdruid requested a review from mortbopet June 19, 2023 22:46
@sequencer
Copy link
Contributor

Using mux-based to implement a LUT might be inefficient for ASIC designs, it that possible to be configured to add other implementations which may be designed for ASIC specifically in the future?

@teqdruid
Copy link
Contributor Author

Using mux-based to implement a LUT might be inefficient for ASIC designs, it that possible to be configured to add other implementations which may be designed for ASIC specifically in the future?

Yes. We'd add a pass option to specify a lowering style -- mux vs casez vs primitive vs .... Casez is planned.

@sequencer
Copy link
Contributor

Sounds great, but if not having dont care bits, would casez version be efficient? Or the dc will get implemented in the future?

@teqdruid
Copy link
Contributor Author

teqdruid commented Jun 20, 2023

casez with selected/computed ? bits is required to get us the xprop behavior we want. See the June 7th ODM for that discussion.

@sequencer
Copy link
Contributor

Thanks for information!

Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a lowering with the Comb dialect in general makes sense to me. Just left a few comments. I'm not exactly sure what you need to lower to in order to achieve what you want, but the general implementation looks good.

include/circt/Dialect/Comb/Passes.td Outdated Show resolved Hide resolved
lib/Dialect/Comb/Transforms/LowerComb.cpp Outdated Show resolved Hide resolved
lib/Dialect/Comb/Transforms/LowerComb.cpp Outdated Show resolved Hide resolved
@teqdruid
Copy link
Contributor Author

#5433 <- pass boilerplate PR

@teqdruid
Copy link
Contributor Author

Having a lowering with the Comb dialect in general makes sense to me. Just left a few comments. I'm not exactly sure what you need to lower to in order to achieve what you want, but the general implementation looks good

A tree of trinary operations works as does a casez with ? used in places where an x input doesn't cause the output to change. Or a casex, though there are other problems with casex and most tools issue a warning on all casex statements. That's all we've found so far.

@teqdruid teqdruid requested a review from mikeurbach June 20, 2023 22:07
@teqdruid
Copy link
Contributor Author

Since I've already gotten a bunch of feedback, I'll be relying on post-review comments for further revisions.

@teqdruid teqdruid merged commit 3153652 into main Jun 20, 2023
5 checks passed
@teqdruid teqdruid deleted the dev/teqruid/tt-lower-mux branch August 9, 2023 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Comb Involving the `comb` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants