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

Remove GenerateSimFiles and use make instead #879

Merged
merged 9 commits into from
May 13, 2021
Merged

Conversation

abejgonzalez
Copy link
Contributor

Related issue: #872

Type of change: other enhancement

Impact: other

Release Notes
By using make we can track the files created by GenerateSimFiles and properly move them / update them on changes. This additionally comes with a bonus of removing 1 SBT call.

@timsnyder

sims/common-sim-flags.mk Outdated Show resolved Hide resolved
sims/vcs/Makefile Outdated Show resolved Hide resolved
@abejgonzalez abejgonzalez self-assigned this May 6, 2021
firrtl.FileUtils.makeDirectory("./bootrom/")
writeResource("/testchipip/bootrom/bootrom.rv64.img", "./bootrom/")
writeResource("/testchipip/bootrom/bootrom.rv32.img", "./bootrom/")
writeResource("/bootrom/bootrom.img", "./bootrom/")
Copy link
Contributor

Choose a reason for hiding this comment

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

What was this /bootrom/bootrom.img in the old code? I didn't completely understand where it was used in Chipyard before but I knew it wouldn't interfere with what I was doing in FireSim. I'm only asking because I don't see where this is being maintained in the new makefile version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. This is Rocket Chips bootrom file: https://github.com/ucb-bar/chipyard/blob/dev/generators/utilities/src/main/resources/bootrom. If you want to use the config fragment in RC (https://github.com/chipsalliance/rocket-chip/blob/b888d0d0eb9975344292d695072e7a9b438cefbb/src/main/scala/subsystem/Configs.scala#L40) you need to have the RC bootrom reside at the top-level in CHIPYARD/bootrom/bootrom.img. Ideally RC does what CY does and uses TargetDirKey... but for now, this should suffice.

Copy link
Contributor Author

@abejgonzalez abejgonzalez May 6, 2021

Choose a reason for hiding this comment

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

I should also note... that 99% of the time people will not use the RC bootrom since the CY one does the same thing for the most part so I'm inclined to ignore this bootrom and keep the makefiles clean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignoring it seems fine to me if they do the same thing. I didn't realize that's where it was coming from or I would have tried to address that in the PR where I added the TargetDirKey.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think they quite do the exact same thing anymore. There are some features we support, which the RC bootrom does not, and vice versa.

We should always use the CY bootrom though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the past we had a SUB_PROJECT=rocketchip which setup things to explicitly use RC collateral and not any CY collateral. However, we have drifted from that over time. So I think we can just ignore the RC bootrom.

@@ -21,3 +21,6 @@ SIM_LDFLAGS = \
-lfesvr \
-ldramsim \
$(EXTRA_SIM_LDFLAGS)

SIM_FILE_REQS += \
$(ROCKETCHIP_RSRCS_DIR)/vsrc/EICG_wrapper.v
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: If chipsalliance/rocket-chip#2810 goes through then this is unnecessary.

Copy link
Contributor

@timsnyder-siv timsnyder-siv left a comment

Choose a reason for hiding this comment

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

Thanks for reminding me to look back through this.

SIM_FILE_REQS += \
$(ROCKETCHIP_RSRCS_DIR)/vsrc/EICG_wrapper.v

# copy files but ignore *.h files in *.f (match vcs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you trying to match vcs here or are you assuming that all FPGA synthesis tools will support +incdir like VCS does and you can do the same thing as you're doing for VCS? If you're really trying to match VCS, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This matches what GenerateSimFiles did in the past (filter out headers). I guess another justification for doing this is that #871 is using VCS to do FPGA-level simulation.

sims/vcs/Makefile Outdated Show resolved Hide resolved
sims/verilator/Makefile Outdated Show resolved Hide resolved
sims/verilator/Makefile Outdated Show resolved Hide resolved
fpga/Makefile Outdated Show resolved Hide resolved
.settings(libraryDependencies ++= rocketLibDeps.value)
.settings(commonSettings)

lazy val utilities = (project in file("generators/utilities"))
.sourceDependency(testchipip, testchipipLib)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Everything I see this I have to remind myself why we need this. Glad to see it getting removed

@jerryz123 jerryz123 self-requested a review May 13, 2021 04:51
@abejgonzalez abejgonzalez merged commit 45f63a1 into dev May 13, 2021
@abejgonzalez abejgonzalez deleted the remove-gen-sim-files branch October 7, 2021 17:48
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.

3 participants