Skip to content

Retirement refactor#548

Merged
tilk merged 6 commits into
kuznia-rdzeni:masterfrom
awariac:piotro888/retirement-refactor
Dec 21, 2023
Merged

Retirement refactor#548
tilk merged 6 commits into
kuznia-rdzeni:masterfrom
awariac:piotro888/retirement-refactor

Conversation

@awariac
Copy link
Copy Markdown
Member

@awariac awariac commented Dec 20, 2023

CONTROVERSY ALERT ❗

I started working on speculation and saw a pattern in Retirement. I refactored it as FSM that calls python methods!
This way we can eliminate:

  • All of the ugly sync/comb duo control signals - python functions that call Methods are executed under If from any state. When implementing misprediction interrupt I would need to add another pair of signalling like that. (also turns out there was previously probably a bug, because rrat call only used sync side_fx)
  • Forwarders - now potentially (almost all) blocking methods are called from separate Transactions.
  • Conditions in Ifs simplified

I think that logic is also way easier to understand with clear separation of retire/flush and state of what the retirement currently is doing.

Unfortunately the limitation that methods can be called only once from transaction, breaks fun here. It would look even nicer without it see commit. And MethodBrancher cannot be used in this case.

For me it looks great. What do you think about it?
Also it would be nice to review/merege it quickly, as my further work on speculation depends on this (it would be ugly and bug-prone otherwise)

@awariac awariac added the refactor Doesn't change functionality, but makes stuff nicer label Dec 20, 2023
@awariac
Copy link
Copy Markdown
Member Author

awariac commented Dec 20, 2023

There seems to be loss in benchmark IPC for some reason. Needs investigation

@awariac
Copy link
Copy Markdown
Member Author

awariac commented Dec 20, 2023

I see, FRAT is still called under one condition form main Retirement. I will probably re-introduce one Forwarder, maybe there is some better solution?

@tilk
Copy link
Copy Markdown
Member

tilk commented Dec 21, 2023

I see, FRAT is still called under one condition form main Retirement. I will probably re-introduce one Forwarder, maybe there is some better solution?

Maybe try using condition? This would split that transaction into two.

@awariac
Copy link
Copy Markdown
Member Author

awariac commented Dec 21, 2023

Maybe try using condition? This would split that transaction into two.

Great! It also fixed problem with call to free_phys_reg. Thanks!
I will check the IPC in a moment to be sure

EDIT: IPC is good

@awariac
Copy link
Copy Markdown
Member Author

awariac commented Dec 21, 2023

oops, there is a problem with generating graph and condtion :(. [There is also graph syntax error on master again]

@tilk
Copy link
Copy Markdown
Member

tilk commented Dec 21, 2023

oops, there is a problem with generating graph and condtion :(. [There is also graph syntax error on master again]

The issue is in get_caller_class_name, I'll prepare a fix soon. Edit: #550

@tilk
Copy link
Copy Markdown
Member

tilk commented Dec 21, 2023

The Fmax is still pretty bad, but I imagine that #545 can still fix it?

@awariac
Copy link
Copy Markdown
Member Author

awariac commented Dec 21, 2023

Fmax issue is unrelated to this PR, it depends only on critical path from JBUnit to ExcepionCauseRegister.
IPC will be back to best value also after merge #545, becaue confilcts are removed here. It will not affect this Retirement code, because stall method is called from exclusive Transaction

@tilk tilk merged commit dc55268 into kuznia-rdzeni:master Dec 21, 2023
github-actions Bot pushed a commit that referenced this pull request Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Doesn't change functionality, but makes stuff nicer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants