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

Make help target and VCS/Verilator Cleanup #650

Merged
merged 23 commits into from
Aug 21, 2020
Merged

Make help target and VCS/Verilator Cleanup #650

merged 23 commits into from
Aug 21, 2020

Conversation

abejgonzalez
Copy link
Contributor

@abejgonzalez abejgonzalez commented Aug 18, 2020

Related issue: #562

Type of change: other enhancement

Impact: other

Release Notes
This is a split of PR #562 with Makefile change minus the Multi-threading support. It includes changes to VCS so that it aligns with the same changes done to Verilator.

sims/vcs/Makefile Outdated Show resolved Hide resolved
sims/vcs/Makefile Outdated Show resolved Hide resolved
sims/vcs/Makefile Outdated Show resolved Hide resolved
sims/vcs/Makefile Outdated Show resolved Hide resolved
common.mk Outdated Show resolved Hide resolved
vlsi/Makefile Outdated Show resolved Hide resolved
@abejgonzalez
Copy link
Contributor Author

Ok the Chipyard simulation side of this should be good to review. I checked VCS simulation and Verilator simulations. Im trying to check Hammer sim right now.

Copy link
Contributor

@colinschmidt colinschmidt left a comment

Choose a reason for hiding this comment

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

A lot of small details.

common.mk Outdated Show resolved Hide resolved
common.mk Outdated Show resolved Hide resolved
common.mk Outdated Show resolved Hide resolved
common.mk Outdated Show resolved Hide resolved
common.mk Outdated Show resolved Hide resolved
variables.mk Outdated Show resolved Hide resolved
vcs.mk Outdated
# gcc configuration/optimization
#----------------------------------------------------------------------------------------
# -flto slows down compilation on small-memory and breaks on firesim-manager
CMODE := -O3 -fbranch-probabilities -march=native
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did this come from? Is it known to always be better than the default? Is it optimizing for compilation time or runtime? Is that what we want?

Copy link
Contributor Author

@abejgonzalez abejgonzalez Aug 19, 2020

Choose a reason for hiding this comment

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

This was copying what Verilator did (though to make Verilator not segfault I just kept the -O3). It is optimizing the runtime. The default is nothing (aka use -0 with nothing else).

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it actually improve runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dunno. I can run the bmark-tests on Rocket with and without the CMODE flags. Im trying to get Hammer sim tested 1st.

Copy link
Contributor Author

@abejgonzalez abejgonzalez Aug 20, 2020

Choose a reason for hiding this comment

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

With CMODE flags:
Compile: 36s
Runtime for all bmark tests (serially): 7m 36s

Without CMODE flags:
Compile: 35s
Runtime for all bmark tests (serially): 7m 34s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While this doesn't result in a large runtime difference, I think it should stay if we end up consolidating flags across VCS and Verilator.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine. It is worth noting that adding code that doesn't affect things is confusing, but if this proves that we can merge the flags in the future I'm fine with leaving it in.

vcs.mk Show resolved Hide resolved
vcs.mk Show resolved Hide resolved
vcs.mk Show resolved Hide resolved
sims/verilator/Makefile Outdated Show resolved Hide resolved
common.mk Outdated Show resolved Hide resolved
sims/vcs/Makefile Outdated Show resolved Hide resolved
@abejgonzalez
Copy link
Contributor Author

Ok. I just got around to checking Hammer Sim. I got the simulator to compile successfully but I am getting other RISCV toolchains problems (causing wrong TSI commands to be sent). I think that is a problem with my environment though not the PR.

@colinschmidt
Copy link
Contributor

I filed issues for the things I suggested be updated in new PRs. I believe the HammerSim changes should work should I'm ok merging once CI passes. We're not close to a release now, so we have plenty of time to dog food the changes anyways.

Copy link
Contributor

@colinschmidt colinschmidt left a comment

Choose a reason for hiding this comment

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

LGTM

@abejgonzalez abejgonzalez merged commit fd0bbb4 into dev Aug 21, 2020
@abejgonzalez abejgonzalez deleted the make-help branch August 21, 2020 06:17
This was referenced Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants