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

[Arc] Simulation specific optimization for comb.icmp #5198

Merged
merged 1 commit into from
May 15, 2023

Conversation

maerhart
Copy link
Member

On the HW/Comb level, the comb.concat operation is basically considered free. However, in simulation it is quite expensive. Instead of concatenating integers of the same width, we can do the AND/OR directly and do a reduction on the smaller result integer, effectively cutting the number of assembly instructions produced to almost half.

@maerhart maerhart added the Arc Involving the `arc` dialect label May 13, 2023
Copy link
Contributor

@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

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

Clever! LGTM

@maerhart maerhart force-pushed the dev/maerhart/arc-canonicalizer branch from 0b19c85 to 0cbc869 Compare May 14, 2023 10:19
@maerhart maerhart force-pushed the dev/maerhart/arc-icmp-pattern branch from c4fc05a to 2a8d650 Compare May 14, 2023 10:32
@maerhart maerhart force-pushed the dev/maerhart/arc-canonicalizer branch from 0cbc869 to 77b0616 Compare May 14, 2023 10:36
@maerhart maerhart force-pushed the dev/maerhart/arc-icmp-pattern branch from 2a8d650 to ba72569 Compare May 14, 2023 10:37
@maerhart
Copy link
Member Author

Some statistics with and without the ICMP pattern.

Design Total Instr. w/o Total Instr. w % Stack Instr. w/o Stack Instr. w %
Riscinator 6084 3235 53.2 1471 941 63.0
Rocket 127622 122454 96.0 33719 32627 96.8

Designs from github.com/circt/arc-tests
Stack Instr measured with: objdump -d design.o | grep -E "push|pop|rsp|rbp" | wc -l
Total Instr measured with objdump -d design.o | wc -l

@fabianschuiki
Copy link
Contributor

Holy cow, I didn't expect this pattern to be that impactful. Very cool stuff!

Base automatically changed from dev/maerhart/arc-canonicalizer to main May 15, 2023 19:30
@maerhart maerhart force-pushed the dev/maerhart/arc-icmp-pattern branch from ba72569 to 8d0df2d Compare May 15, 2023 19:33
@maerhart maerhart merged commit 26d1d3f into main May 15, 2023
@maerhart maerhart deleted the dev/maerhart/arc-icmp-pattern branch May 15, 2023 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arc Involving the `arc` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants