FireSim x Chipyard Decoupled #1984

abejgonzalez · 2024-08-09T00:05:39Z

Originally FireSim and Chipyard were coupled sharing things like (1) Scala sources (and SBT sources) (2) Conda environments (3) Bridges (...) and more. However, this PR decouples FireSim from Chipyard s.t. all "target" (ie. DUT) specific features are located in Chipyard (ie. things like FireSim UART bridges, NIC bridges, etc) and FireSim provides a hook to Chipyard so that I can run the FireSim makefile itself.

This includes the following:

Removes existing FireSim CI tests run in chipyard-run-tests.yml (in the past, we directly invoked the FireSim compiler to test simple RISC-V tests) in favor of new CI run during chipyard-full-flow.yml (Scala-based testing moved from FireSim).
Adds a firesim-staging simulator area where there is a FireSim symlink that points to FireSim when FireSim is top.
In build.sbt abstracts the Scala test dependency
Updates common.mk s.t. it points to the right FireSim sources.
Separates out the firechip area into 3 SBT projects. core (FireSim top-level harness and target-specific bridges), bridge-interfaces (target bridge interfaces that are copied into FireSim), firesim-only (the FireSim-only side of target-specific bridges).

Chipyard documentation is minimally changed (FireSim documentation needs to be updated).

Related PRs / Issues:

Type of change:

Bug fix
New feature
Other enhancement

Impact:

RTL change
Software change (RISC-V software)
Build system change
Other

Contributor Checklist:

Did you set main as the base branch?
Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
Did you state the type-of-change/impact?
Did you delete any extraneous prints/debugging code?
Did you mark the PR with a changelog: label?
(If applicable) Did you update the conda .conda-lock.yml file if you updated the conda requirements file?
(If applicable) Did you add documentation for the feature?
(If applicable) Did you add a test demonstrating the PR?

(If applicable) Did you mark the PR as Please Backport?

build.sbt

common.mk

generators/firechip/core/src/main/makefrag/firesim/build.mk

abejgonzalez · 2024-08-19T19:12:47Z

build.sbt

  .settings(commonSettings)
  .settings(chiselSettings)
+// TODO: AJG: ^ Fix


I'll fix this in another PR.

abejgonzalez · 2024-08-20T05:52:57Z

This is ready for review (passes all CI except for commit-on-master).

jerryz123

I think I understand why bridges are in here - they are target-specific.

It is not as clear to me why the .mk fragments are here. Aren't those core parts of firesim's build system that should remain with firesim? In what ways are they target-specific?

abejgonzalez · 2024-08-20T16:22:26Z

I think I understand why bridges are in here - they are target-specific.

It is not as clear to me why the .mk fragments are here. Aren't those core parts of firesim's build system that should remain with firesim? In what ways are they target-specific?

You can think of them similarly to vcs.mk or the Verilator Makefile. They are the hooks that FireSim provides to a downstream project (i.e. Chipyard) to configure it's make system to do the right thing. Specifically, they indicate how to regenerate the FIRRTL/annos, define extra target specific simulation flags, define default make flags, point to bridge CC files, define default driver arguments, and more. All of these things are target-specific.

With VCS/Verilator in Chipyard we directly invoke the Makefile in the respective folder which calls the tools. FireSim is a bit different since there is the FireSim manager. The manager call's FireSim's firesim/sim/Makefile which points to the makefrag hooks put here, allowing FireSim to do the right thing.

jerryz123 · 2024-08-20T17:42:43Z

Ok I see, so this is a side-effect of fs's Python manager invoking makefiles that describe construction of target-specific things.

I think a longer-term goal would be to avoid having the interface to firesim be through make fragments, but this arrangement is fine fo rnow.

abejgonzalez · 2024-08-20T17:55:02Z

Ok I see, so this is a side-effect of fs's Python manager invoking makefiles that describe construction of target-specific things.

I think a longer-term goal would be to avoid having the interface to firesim be through make fragments, but this arrangement is fine fo rnow.

I agree. But I think this is sufficient for now (like you said)

generators/firechip/core/src/main/cc/bridges/cospike/mem_pool.cc

jerryz123 · 2024-08-20T18:12:29Z

Separates out the firechip area into 3 SBT projects. core (FireSim top-level harness and target-specific bridges), bridge-interfaces (target bridge interfaces that are copied into FireSim), firesim-only (the FireSim-only side of target-specific bridges).

I'm not sure I'm a fan of firechip.core, I found myself needing to refer to this comment to remind myself that firechip.core contains target-specific things, the "core" throws me off. How about "firechip.target"? Basically everything that used to be firechip is now in here right?

abejgonzalez · 2024-08-20T18:40:26Z

Separates out the firechip area into 3 SBT projects. core (FireSim top-level harness and target-specific bridges), bridge-interfaces (target bridge interfaces that are copied into FireSim), firesim-only (the FireSim-only side of target-specific bridges).

I'm not sure I'm a fan of firechip.core, I found myself needing to refer to this comment to remind myself that firechip.core contains target-specific things, the "core" throws me off. How about "firechip.target"? Basically everything that used to be firechip is now in here right?

I'm avoiding calling things target because SBT generates the target directory for built Scala classes under what would originally be core (i.e. firechip/target/target for Scala classes rather than firechip/target/src/... for everything else)(assuming you name the subdirectory after the package name). I didn't spend much time thinking of a better name than core to avoid this. Happy to change the package path though if you have another rec. And yes, this core package contains all the old code of FireChip.

jerryz123 · 2024-08-20T18:50:06Z

Ah, that conflict is a very good point.
How about:

firechip.chip
firechip.bridges.interfaces
firechip.bridges.implementations

abejgonzalez · 2024-08-20T18:58:36Z

How about this instead:

firechip.core.firesim -> firechip.chip under folder firechip/chip/src/main/scala/*.scala
firechip.core.bridges -> firechip.bridges.implementations (technically these are stubs not implementations since the implementations live on the FireSim-side - I think stubs would be better here) under folder firechip/bridgesimplementations/src/main/scala/*.scala
firechip.bridgeinterfaces -> firechip.bridges.interfaces under folder firechip/bridgesinterfaces/src/main/scala/*.scala
firechip.firesimonly.bridges -> firechip.firesimonly under folder firechip/firesimonly/src/main/scala/*.scala

The bridge interfaces should be in a separate SBT project / src/main/scala area so that it is easy to symlink in the Makefrags (ideally).

Edit: Can also combine the package path a bit so things are consistent: firechip.bridgesinterfaces and firechip.bridgesimplementations

joonho3020 · 2024-08-20T21:45:54Z

I find the name firesimonly a bit confusing.
What about

firechip.bridges.io: for the IO ports & case classes
firechip.bridges.hooks: for the annotation generator
firechip.bridges.implementations : for the HostBridge RTL implementation

abejgonzalez · 2024-08-20T21:53:58Z

I find the name firesimonly a bit confusing. What about

firechip.bridges.io: for the IO ports & case classes firechip.bridges.hooks: for the annotation generator firechip.bridges.implementations : for the HostBridge RTL implementation

IMO interfaces is better than io since IMO io generally denotes just Chisel IOs. I think hooks is ok (I think it's about as good as stubs). I like keeping firesimonly to strongly tell users "this folder is not compiled with Chipyard" directly from looking at the folder structure. Why do you think its confusing?

joonho3020 · 2024-08-20T22:02:51Z

I would like the name to convey that these are bridge RTL implementations which are elaborated when midas runs. firesim-only doesn't describe what these files are... What about midas-input-impls?

jerryz123

Overall this looks fine. Once we resolve the package naming, I think this can be merged

abejgonzalez · 2024-08-21T01:06:21Z

How about this instead:

firechip.core.firesim -> firechip.chip under folder firechip/chip/src/main/scala/*.scala
firechip.core.bridges -> firechip.bridgeimplementations (technically these are stubs not implementations since the implementations live on the FireSim-side - I think stubs would be better here) under folder firechip/bridgeimplementations/src/main/scala/*.scala
firechip.bridgeinterfaces -> firechip.bridgeinterfaces under folder firechip/bridgesinterfaces/src/main/scala/*.scala
firechip.firesimonly.bridges -> firechip.goldengateimplementations under folder firechip/goldengateimplementations/src/main/scala/*.scala

Ok @joonho3020 how about this?

joonho3020 · 2024-08-21T01:11:07Z

I don't want to nitpick too much, but now firechip.core.bridges -> firechip.bridgeimplementations is confusing. Can we just use bridgestubs like you mentioned?

abejgonzalez · 2024-08-21T01:16:37Z

Nitpick now or forever hold your peace.

firechip.core.firesim -> firechip.chip under folder firechip/chip/src/main/scala/*.scala
firechip.core.bridges -> firechip.bridgestubs under folder firechip/bridgestubs/src/main/scala/*.scala
firechip.bridgeinterfaces -> firechip.bridgeinterfaces under folder firechip/bridgeinterfaces/src/main/scala/*.scala
firechip.firesimonly.bridges -> firechip.goldengateimplementations under folder firechip/goldengateimplementations/src/main/scala/*.scala

How about now?

joonho3020 · 2024-08-21T02:15:51Z

Thanks, I like it

abejgonzalez · 2024-08-22T03:11:06Z

Disabling CI since the latest commit(s) shouldn't affect anything.

abejgonzalez added the changelog:omit label Aug 9, 2024

abejgonzalez self-assigned this Aug 9, 2024

abejgonzalez added the DO NOT MERGE label Aug 9, 2024

abejgonzalez force-pushed the split-v1 branch from f57e72f to 61b5ebc Compare August 9, 2024 00:16

abejgonzalez added the ci:disable label Aug 9, 2024

abejgonzalez closed this Aug 9, 2024

abejgonzalez reopened this Aug 9, 2024

abejgonzalez commented Aug 14, 2024

View reviewed changes

build.sbt Outdated Show resolved Hide resolved

abejgonzalez commented Aug 14, 2024

View reviewed changes

build.sbt Show resolved Hide resolved

abejgonzalez commented Aug 14, 2024

View reviewed changes

common.mk Show resolved Hide resolved

abejgonzalez commented Aug 14, 2024

View reviewed changes

generators/firechip/core/src/main/makefrag/firesim/build.mk Outdated Show resolved Hide resolved

abejgonzalez force-pushed the split-v1 branch from 4398a10 to 64f0b33 Compare August 19, 2024 06:09

abejgonzalez added changelog:changed and removed ci:disable changelog:omit labels Aug 19, 2024

abejgonzalez closed this Aug 19, 2024

abejgonzalez reopened this Aug 19, 2024

abejgonzalez commented Aug 19, 2024

View reviewed changes

abejgonzalez requested a review from jerryz123 August 19, 2024 19:15

abejgonzalez changed the title ~~WIP: FireSim x Chipyard Decoupled~~ FireSim x Chipyard Decoupled Aug 19, 2024

abejgonzalez mentioned this pull request Aug 19, 2024

FireSim x Chipyard Decoupled firesim/firesim#1791

Merged

11 tasks

abejgonzalez force-pushed the split-v1 branch from 7e7ebc0 to 27da7c2 Compare August 19, 2024 21:10

abejgonzalez marked this pull request as ready for review August 19, 2024 21:53

abejgonzalez removed the DO NOT MERGE label Aug 19, 2024

jerryz123 reviewed Aug 20, 2024

View reviewed changes

generators/firechip/core/src/main/cc/bridges/cospike/mem_pool.cc Outdated Show resolved Hide resolved

abejgonzalez added 5 commits August 20, 2024 15:04

Split FireSim from Chipyard (and vice versa)

f23b1d2

Bump FireSim

a32d930

Disable Verilator/Vivado in FireChip CI

5708a56

Reenable ResetBridge + AutoCounter covers in FireChip

31c75c1

Bump FireSim

6fd75a3

abejgonzalez force-pushed the split-v1 branch from ad1a047 to 6fd75a3 Compare August 20, 2024 22:04

jerryz123 reviewed Aug 20, 2024

View reviewed changes

jerryz123 approved these changes Aug 21, 2024

View reviewed changes

abejgonzalez added 5 commits August 20, 2024 23:32

Rework package paths

73dbdf1

Bump FireSim

760d941

Fix docs

90342e1

Fix CI path

438186f

Bump FireSim

07ae28a

abejgonzalez added the ci:disable label Aug 22, 2024

abejgonzalez merged commit 87edef0 into main Aug 22, 2024
10 of 58 checks passed

abejgonzalez mentioned this pull request Aug 26, 2024

Add stubs to enable SFC passes #1904

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FireSim x Chipyard Decoupled #1984

FireSim x Chipyard Decoupled #1984

abejgonzalez commented Aug 9, 2024 •

edited

Loading

abejgonzalez Aug 19, 2024 •

edited

Loading

abejgonzalez commented Aug 20, 2024

jerryz123 left a comment

abejgonzalez commented Aug 20, 2024 •

edited

Loading

jerryz123 commented Aug 20, 2024

abejgonzalez commented Aug 20, 2024

jerryz123 commented Aug 20, 2024

abejgonzalez commented Aug 20, 2024 •

edited

Loading

jerryz123 commented Aug 20, 2024

abejgonzalez commented Aug 20, 2024 •

edited

Loading

joonho3020 commented Aug 20, 2024

abejgonzalez commented Aug 20, 2024

joonho3020 commented Aug 20, 2024

jerryz123 left a comment

abejgonzalez commented Aug 21, 2024

joonho3020 commented Aug 21, 2024

abejgonzalez commented Aug 21, 2024

joonho3020 commented Aug 21, 2024 •

edited

Loading

abejgonzalez commented Aug 22, 2024

FireSim x Chipyard Decoupled #1984

FireSim x Chipyard Decoupled #1984

Conversation

abejgonzalez commented Aug 9, 2024 • edited Loading

abejgonzalez Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

abejgonzalez commented Aug 20, 2024

jerryz123 left a comment

Choose a reason for hiding this comment

abejgonzalez commented Aug 20, 2024 • edited Loading

jerryz123 commented Aug 20, 2024

abejgonzalez commented Aug 20, 2024

jerryz123 commented Aug 20, 2024

abejgonzalez commented Aug 20, 2024 • edited Loading

jerryz123 commented Aug 20, 2024

abejgonzalez commented Aug 20, 2024 • edited Loading

joonho3020 commented Aug 20, 2024

abejgonzalez commented Aug 20, 2024

joonho3020 commented Aug 20, 2024

jerryz123 left a comment

Choose a reason for hiding this comment

abejgonzalez commented Aug 21, 2024

joonho3020 commented Aug 21, 2024

abejgonzalez commented Aug 21, 2024

joonho3020 commented Aug 21, 2024 • edited Loading

abejgonzalez commented Aug 22, 2024

abejgonzalez commented Aug 9, 2024 •

edited

Loading

abejgonzalez Aug 19, 2024 •

edited

Loading

abejgonzalez commented Aug 20, 2024 •

edited

Loading

abejgonzalez commented Aug 20, 2024 •

edited

Loading

abejgonzalez commented Aug 20, 2024 •

edited

Loading

joonho3020 commented Aug 21, 2024 •

edited

Loading