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

Split register info class into multiple classes; one per register file #320

Merged
merged 17 commits into from
Nov 26, 2023

Conversation

raccog
Copy link
Contributor

@raccog raccog commented Nov 14, 2023

Create RegInfoInterface as an abstract class to interface into a single register file. Return a vector of all the RegInfoInterfaces implemented by an ISA.

Also refactored any code that indexes a register so that the register's file type is used in the index.

This includes the command line option --reginit. It now requires the register file type to be specified for each initialized register set. It can be used multiple times to initialize multiple register files. Example:

./Ripes --reginit gpr:1=1,20=0xdeadbeef --reginit csr:1=0xf

@mortbopet
Copy link
Owner

I understand the intention of the change, but I'm not sure think that (as far as i understand) essentially merging multiple register infos in e.g. RegInfoSet is a proper representation. Each register file is distinct, and i think all places which care about providing a representation of the register files should represent each one as distinct, and not lump them together.

I'd be more in favor of ISAInfoBase returning a list of RegInfoInterface, which each UI element then can use to provide distinct views of each register file: std::vector<RegInfoInterface*> ISAInfoBase::getRegisterInfos()

@raccog
Copy link
Contributor Author

raccog commented Nov 15, 2023

I'd be more in favor of ISAInfoBase returning a list of RegInfoInterface, which each UI element then can use to provide distinct views of each register file: std::vector<RegInfoInterface*> ISAInfoBase::getRegisterInfos()

@mortbopet This sounds good to me since returning a std::vector<RegInfoInterface*> would serve the same purpose as RegInfoSet by providing a way to iterate over each available register from each register file that an ISA implements.

@raccog raccog marked this pull request as draft November 16, 2023 23:27
src/cli/clioptions.cpp Outdated Show resolved Hide resolved
@raccog raccog marked this pull request as ready for review November 18, 2023 21:32
Copy link
Owner

@mortbopet mortbopet left a comment

Choose a reason for hiding this comment

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

LGTM after these things are addressed.

src/cli/clioptions.cpp Outdated Show resolved Hide resolved
src/cli/clioptions.cpp Outdated Show resolved Hide resolved
src/isa/isainfo.h Outdated Show resolved Hide resolved
src/isa/isainfo.h Outdated Show resolved Hide resolved
src/registerinitializationwidget.cpp Outdated Show resolved Hide resolved
src/registermodel.cpp Outdated Show resolved Hide resolved
src/registermodel.cpp Outdated Show resolved Hide resolved
test/tst_cosimulate.cpp Outdated Show resolved Hide resolved
@mortbopet mortbopet merged commit 357f796 into mortbopet:master Nov 26, 2023
6 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