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

Add muldiv_c and muxadd peepopts #4740

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

akashlevy
Copy link
Contributor

@akashlevy akashlevy commented Nov 13, 2024

What are the reasons/motivation for this change?

This PR adds two peep-hole optimizations that improve netlist quality:

  1. muldiv_c: y = (a * b_const) / c_const ===> a * eval(b_const / c_const) if b_const % c_const == 0. This implements basic constant propagation for multipliers and dividers with divisible constants.
  2. muxadd: y = s ? (a + b) : a ===> y = a + (s ? b : 0). This provides useful restructuring for improving logic optimization. s ? b : 0 can be optimized to a bitwise AND in subsequent optimizations.

Explain how this is achieved.

  • passes/pmgen/peepopt_muldiv_c.pmg: muldiv_c peepopt
  • passes/pmgen/peepopt_muxadd.pmg: muxadd peepopt
  • passes/pmgen/peepopt.cc: Add to docs and run peepopts during peepopt pass
  • passes/pmgen/Makefile.inc: Add the new sources

If applicable, please suggest to reviewers how they can test the change.

  • YosysHQ to review source code and provide feedback/edits as necessary
  • YosysHQ to construct test plan of 15-20 small-medium test cases per peepopt
  • Silimate to review test plan and sign off
  • YosysHQ to write test cases according to test plan and add to regression
  • YosysHQ to provide formal equivalence checking scripts with equiv_opt or FOSS eqy
  • Silimate to sign off on test case implementation
  • YosysHQ to merge PR

Copy link
Member

@povik povik left a comment

Choose a reason for hiding this comment

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

Hi Akash, I looked over the muxadd pattern

passes/pmgen/peepopt_muxadd.pmg Outdated Show resolved Hide resolved
std::swap(add_a, add_b);
endcode

match mux
Copy link
Member

Choose a reason for hiding this comment

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

We can also match on the case of swapped mux inputs

match mux
// Select mux of form s ? (a + b) : a, allow leading 0s when A_WIDTH != Y_WIDTH
select mux->type == $mux
index <SigSpec> port(mux, \A) === SigSpec({Const(State::S0, GetSize(add_y)-GetSize(add_a)), add_a})
Copy link
Member

Choose a reason for hiding this comment

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

This should be zero-padded if !param(add, \A_SIGNED).bool() and sign-extended otherwise (provided add_a is the A input)

Copy link
Member

Choose a reason for hiding this comment

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

Also I am not sure this doesn't crash on add_y < add_a which I don't think we rule out anywhere

passes/pmgen/peepopt_muxadd.pmg Show resolved Hide resolved
mux->setPort(\A, Const(State::S0, GetSize(add_b)));
mux->setPort(\B, add_b);
mux->setPort(\Y, mid);
add->setPort(\B, mid);
Copy link
Member

Choose a reason for hiding this comment

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

Wrong side of the adder if the swap above was hit (we need to use add_a_id instead)

Copy link
Member

Choose a reason for hiding this comment

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

Err, more like add_b_id

@akashlevy
Copy link
Contributor Author

Thanks for the feedback on muxadd. I'll take a look today

std::swap(a, b_const);
endcode

match div
Copy link
Member

Choose a reason for hiding this comment

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

No check that the divider and multiplier are connected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants