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

Core class cleanup #387

Merged
merged 9 commits into from
Feb 16, 2024
Merged

Core class cleanup #387

merged 9 commits into from
Feb 16, 2024

Conversation

FinnWilkinson
Copy link
Contributor

@FinnWilkinson FinnWilkinson commented Feb 8, 2024

Cleans up the Core classes by moving common member variables into the base class and ensuring the code aligns with the rest of the project.

Closes #356
Fixes #386

@FinnWilkinson FinnWilkinson added code consistency Cleaning up existing code to align with rest of project 0.9.6 Part of SimEng Release 0.9.6 labels Feb 8, 2024
@FinnWilkinson FinnWilkinson self-assigned this Feb 8, 2024
jj16791
jj16791 previously approved these changes Feb 9, 2024
Copy link
Contributor

@ABenC377 ABenC377 left a comment

Choose a reason for hiding this comment

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

Only comment relates to the units used for clock frequency. So far as I can tell, there isn't a good reason for using GHz in some places and Hz in others. In which case, it would seem better to stick to one unit throughout the code base. However, @FinnWilkinson @jj16791 @dANW34V3R @JosephMoore25, please let me know if I'm missing something

src/include/simeng/Core.hh Outdated Show resolved Hide resolved
src/lib/models/emulation/Core.cc Outdated Show resolved Hide resolved
src/lib/models/inorder/Core.cc Outdated Show resolved Hide resolved
src/lib/models/outoforder/Core.cc Outdated Show resolved Hide resolved
@jj16791
Copy link
Contributor

jj16791 commented Feb 12, 2024

Only comment relates to the units used for clock frequency. So far as I can tell, there isn't a good reason for using GHz in some places and Hz in others. In which case, it would seem better to stick to one unit throughout the code base. However, @FinnWilkinson @jj16791 @dANW34V3R @JosephMoore25, please let me know if I'm missing something

You'd be correct in the use cases primarily converting the input GHz value into Hz. A significant reason behind the input being in GHz is that that's the natural unit of measure for modern HPC CPU cores. It's very unnatural to think about the frequency in terms of Hz and we'd need to add comments and notes to ensure that the user diverts from the natural assumptions. We kept it as GHz and got the code to convert it to Hz to make it "easier"/more convenient for the user.

@FinnWilkinson
Copy link
Contributor Author

Only comment relates to the units used for clock frequency. So far as I can tell, there isn't a good reason for using GHz in some places and Hz in others. In which case, it would seem better to stick to one unit throughout the code base. However, @FinnWilkinson @jj16791 @dANW34V3R @JosephMoore25, please let me know if I'm missing something

You'd be correct in the use cases primarily converting the input GHz value into Hz. A significant reason behind the input being in GHz is that that's the natural unit of measure for modern HPC CPU cores. It's very unnatural to think about the frequency in terms of Hz and we'd need to add comments and notes to ensure that the user diverts from the natural assumptions. We kept it as GHz and got the code to convert it to Hz to make it "easier"/more convenient for the user.

Is there a reason for using Hz in the code at all though? We could store clockFrequency_ as a float and in terms of GHz given the usage inside each core is to just divide it by 1e9 again (i.e. put it back from Hz to GHz)

jj16791
jj16791 previously approved these changes Feb 12, 2024
ABenC377
ABenC377 previously approved these changes Feb 12, 2024
JosephMoore25
JosephMoore25 previously approved these changes Feb 13, 2024
dANW34V3R
dANW34V3R previously approved these changes Feb 14, 2024
JosephMoore25
JosephMoore25 previously approved these changes Feb 15, 2024
jj16791
jj16791 previously approved these changes Feb 15, 2024
dANW34V3R
dANW34V3R previously approved these changes Feb 16, 2024
ABenC377
ABenC377 previously approved these changes Feb 16, 2024
@FinnWilkinson FinnWilkinson merged commit f4bd1ef into dev Feb 16, 2024
2 checks passed
@FinnWilkinson FinnWilkinson deleted the core-class-cleanup branch February 21, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.9.6 Part of SimEng Release 0.9.6 code consistency Cleaning up existing code to align with rest of project
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

SimEng/src/lib/Elf.cc missing cstdio Clean Up Core Class
5 participants