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

New core creation #247

Merged
merged 19 commits into from
Sep 16, 2022
Merged

New core creation #247

merged 19 commits into from
Sep 16, 2022

Conversation

jj16791
Copy link
Contributor

@jj16791 jj16791 commented Sep 12, 2022

A new CoreInstance class which creates the core and its associated memory interfaces. main.cc now creates an instance of this class and uses it to generate the SimEng simulation objects.

This change was influenced by the need to remove any dependencies SST had on yaml-cpp due to AppleClang having issues linking them. Through this class, the new SST core wrapper can now use the new CoreINstance class to create all SimEng objects without the need to link against the yaml-ccp external project.

@jj16791 jj16791 added bug Something isn't working enhancement New feature or request labels Sep 12, 2022
@jj16791 jj16791 self-assigned this Sep 12, 2022
@jj16791 jj16791 changed the base branch from main to dev September 12, 2022 14:05
@dANW34V3R dANW34V3R assigned dANW34V3R and unassigned dANW34V3R Sep 12, 2022
Copy link
Contributor

@dANW34V3R dANW34V3R left a comment

Choose a reason for hiding this comment

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

A couple of suggested changes. Will review again once #243 goes through

src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/tools/simeng/main.cc Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/tools/simeng/main.cc Outdated Show resolved Hide resolved
Copy link
Contributor Author

@jj16791 jj16791 left a comment

Choose a reason for hiding this comment

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

Need to add getters for process image shared pointer and its size

Copy link
Contributor

@FinnWilkinson FinnWilkinson left a comment

Choose a reason for hiding this comment

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

Some confusion over current design decisions

src/include/simeng/CoreInstance.hh Outdated Show resolved Hide resolved
src/include/simeng/CoreInstance.hh Outdated Show resolved Hide resolved
src/include/simeng/CoreInstance.hh Outdated Show resolved Hide resolved
src/include/simeng/MemoryInterface.hh Outdated Show resolved Hide resolved
src/include/simeng/CoreInstance.hh Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/tools/simeng/main.cc Outdated Show resolved Hide resolved
configs/a64fx.yaml Outdated Show resolved Hide resolved
configs/a64fx.yaml Outdated Show resolved Hide resolved
src/lib/ModelConfig.cc Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/tools/simeng/main.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@FinnWilkinson FinnWilkinson left a comment

Choose a reason for hiding this comment

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

Looks good to me

src/tools/simeng/main.cc Show resolved Hide resolved
src/include/simeng/CoreInstance.hh Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@dANW34V3R dANW34V3R left a comment

Choose a reason for hiding this comment

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

A couple of minor comments

src/tools/simeng/main.cc Outdated Show resolved Hide resolved
src/include/simeng/CoreInstance.hh Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Show resolved Hide resolved
Copy link
Contributor

@FinnWilkinson FinnWilkinson left a comment

Choose a reason for hiding this comment

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

More recent updates all look good

@rahahahat
Copy link
Contributor

Looks good to me!

Copy link
Contributor

@dANW34V3R dANW34V3R left a comment

Choose a reason for hiding this comment

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

New changes look good. I might change the final comment at a later date

src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
src/include/simeng/CoreInstance.hh Outdated Show resolved Hide resolved
src/lib/CoreInstance.cc Outdated Show resolved Hide resolved
@jj16791 jj16791 merged commit 13f0f83 into dev Sep 16, 2022
jj16791 added a commit that referenced this pull request Oct 17, 2022
This PR introduces a new CoreInstance class. The class supports the creation of a SimEng core model, storing all the relevant simulation objects within shared pointers.

A key factor in this change being introduced was to improve the ease SimEng's interactions with other frameworks e.g. SST.
@jj16791 jj16791 deleted the newCoreCreation branch October 18, 2022 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants