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

Delay flush by one cycle #762

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

tilk
Copy link
Member

@tilk tilk commented Nov 26, 2024

This PR simplifies retirement by delaying the start of flushing. This eliminates one use of condition and makes the flushing signal registered. Looking at benchmarks:

  • IPC is not significantly impacted somehow didn't change at all.
  • Fmax did not visibly improve (sadly), but:
  • The critical path is now different.
  • Transactron reports one less transaction and three less methods.

@tilk tilk added the benchmark Benchmarks should be run for this change label Nov 26, 2024
Copy link

Benchmarks summary

Performance benchmarks

aha-mont64 crc32 minver nettle-sha256 nsichneu slre statemate ud
0.417 (0.000) 0.513 (0.000) 0.337 (0.000) 0.655 (0.000) 0.361 (0.000) 0.290 (0.000) 0.326 (0.000) 0.431 (0.000)

You can view all the metrics here.

Synthesis benchmarks (basic)

Device utilisation: (ECP5) LUTs used as DFF: (ECP5) LUTs used as carry: (ECP5) LUTs used as ram: (ECP5) Max clock frequency (Fmax)
▼ 16263 (-53) ▼ 6276 (-1) ▲ 1460 (+32) 1196 (0) ▲ 56 (+2)

Synthesis benchmarks (full)

Device utilisation: (ECP5) LUTs used as DFF: (ECP5) LUTs used as carry: (ECP5) LUTs used as ram: (ECP5) Max clock frequency (Fmax)
▲ 27509 (+3538) ▼ 8891 (-1) 1822 (0) 1248 (0) ▼ 41 (-2)

@tilk tilk requested a review from Arusekk November 26, 2024 13:10
@tilk tilk added the optimization This is *just* an optimization! label Nov 26, 2024
Comment on lines -183 to +182
# Condition is used to avoid FRAT locking during normal operation
with condition(m) as cond:
with cond(commit):
retire_instr(rob_entry)
with cond():
# Not using default condition, because we want to block if branch is not ready
flush_instr(rob_entry)

m.d.comb += core_flushing.eq(1)
with m.If(commit):
retire_instr(rob_entry)
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid - when instruction is an exception with commit=0 (like synchronous exceptions),
RF is not freed (and not pushed to Free RF), but instruction is retired from ROB, and TRAP_FLUSH would flush next instruction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, you're right - this was not thought out enough. Still, this means we have a blind spot in tests for this.

@piotro888
Copy link
Member

#740 (comment)

@tilk tilk marked this pull request as draft November 26, 2024 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Benchmarks should be run for this change optimization This is *just* an optimization!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants