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

techmap: Add a Kogge-Stone option for $lcu mapping #4298

Merged
merged 5 commits into from
Apr 8, 2024

Conversation

povik
Copy link
Member

@povik povik commented Mar 25, 2024

No description provided.

@povik
Copy link
Member Author

povik commented Mar 25, 2024

You use this with techmap -D KOGGE_STONE, alternatively we can put this into a file of its own as a rule that takes precedence over the default rule, so one would do techmap -map +/techmap.v -map +/kogge-stone.v or similar.

@jamestiotio
Copy link

Hi @povik, I hope you don't mind me chiming in here from a user perspective. The "Kogge-Stone" in the PR title caught my attention. 😄

Putting this into a file of its own would be neater in my opinion. This is mainly because there are various "flavors" of LCUs and tree adders, each with its own benefits and limitations (I assume Kogge-Stone and Brent-Kung are the most popular ones). Allowing users to select different LCU variations by specifying the file that they want seems to be more organized.

This would also allow us to add more variations more easily in the future. We can simply add more files instead of having to fiddle around with multiple conditional directives. This also has the benefit of not having to worry about the order of these directives if the user is the one who specifies the rule precedence.

Just some things to consider for this implementation. Thanks for the great work!

@povik
Copy link
Member Author

povik commented Mar 26, 2024

Hi @jamestiotio, not at all, feel free to chime in. We have discussed this among Yosys maintainers after I opened the PR and the plan now is to go with separate files, like you suggest. So this will get renamed to +/choices/kogge-stone.v. Among other points this has the advantage that one can do synth -extra-map +/choices/kogge-stone.v.

This also has the benefit of not having to worry about the order of these directives if the user is the one who specifies the rule precedence.

It wouldn't work exactly this way, since the rule priority is derived from the name of the rule (e.g. _90_lcu), not the file it's placed in, or the position of that file on the argument list to techmap.

@jamestiotio
Copy link

We have discussed this among Yosys maintainers after I opened the PR and the plan now is to go with separate files, like you suggest.

Nice, that's good to hear!

It wouldn't work exactly this way, since the rule priority is derived from the name of the rule (e.g. _90_lcu), not the file it's placed in, or the position of that file on the argument list to techmap.

Ah got it, I think I misunderstood the rule priority determination. Thanks for the clarification.

@povik povik requested a review from nakengelhardt March 27, 2024 10:09
@povik povik marked this pull request as ready for review March 27, 2024 10:09
@@ -42,7 +42,7 @@ module _80_lcu_kogge_stone (P, G, CI, CO);
g[0] = g[0] | (p[0] & CI);

for (i = 0; i < $clog2(WIDTH); i = i + 1) begin
for (j = 2**i; j < WIDTH; j = j + 1) begin
for (j = WIDTH - 1; j >= 2**i; j = j - 1) begin
Copy link
Member Author

Choose a reason for hiding this comment

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

Surprisingly enough, this passed equivalence testing before the fix

@povik povik merged commit dc74608 into YosysHQ:main Apr 8, 2024
16 checks passed
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