Target-Facing Support for Multiclock Simulation in FireSim #468

davidbiancolin · 2020-03-13T05:15:08Z

This is the chipyard side of the multiclock PR in FireSim. I'm mostly interested in feedback on the chisel API for the clock bridge. How we choose to architect in support for multiple clock domains can easily change in the future.

Related issue: firesim/firesim#441

Type of change: new feature

Release Notes
Support simulation of targets with multiple clock domains in FireSim

jerryz123 · 2020-03-14T05:55:07Z

generators/firechip/src/main/scala/BridgeBinders.scala

-class WithTracerVBridge extends RegisterBridgeBinder({
-  case target: HasTraceIOImp => TracerVBridge(target.traceIO)(target.p)
-})
+class WithTracerVBridge extends Config((_,_,_) => { case InstantiateTracerVBridges => true })


I'm curious why this moves away from the IOBinders approach?
I believe @abejgonzalez is developing the Dromajo widget as another BridgeBinder that attaches to HasTraceIOImp.

I made this change prior to your generator unification work, and i thought it would be nice to move the instantiation closer to the source. I was thiking that ideally the bridge could be instantiated directly in the tile itself perhaps using one of the generation hooks that exists in rocketchip. Then i could just delete the target mixins altogether.

But yes, in light of recent changes, I think it makes sense to continue plumbing this out to the top. I'll switch back to that scheme.

Yes. Jerry is right. Right now the Dromajo Bridge nicely hooks into the TraceIO so keeping that interface would be nice. However, I am not opposed to it changing, I'll just follow what TracerV is doing.

abejgonzalez · 2020-03-16T19:04:53Z

generators/firechip/src/main/scala/TargetConfigs.scala

+  }
+  case BoomCrossingKey => up(BoomCrossingKey, site) map { r =>
+    r.copy(crossingType = RationalCrossing())
+  }


Hmm. I have no idea if the ArianeTile deals with crossings properly... but I think this should include this for now:

case ArianeCrossingKey => up(ArianeCrossingKey, site) map { a => a.copy(crossingType = RationalCrossing()) }

Also nit: change the map from r -> b for BOOM?

abejgonzalez · 2020-03-16T19:17:53Z

generators/firechip/src/main/scala/TargetMixins.scala

@@ -72,3 +78,15 @@ trait CanHaveMultiCycleRegfileImp {
  }
 }

+trait HasFireSimClockingImp extends HasAdditionalClocks {
+  val outer: HasTiles
+  val (tileClock, tileReset) = p(FireSimClockKey).additionalClocks.headOption match {


Can you give an example of assigning different clocks to each of the tiles? Is that supported with this right now? It looks like you have the tiles on one domain and the uncore on another.

colinschmidt

A few questions

colinschmidt · 2020-03-16T22:49:32Z

generators/firechip/src/main/scala/TargetConfigs.scala

@@ -344,3 +346,29 @@ class FireSimTraceGenL2Config extends Config(
    outerLatencyCycles = 50) ++
  new WithTraceGenBridge ++
  new FireSimRocketChipConfig)
+
+
+class WithRationalTiles(multiplier: Int, divisor: Int) extends Config((site, here, up) => {


Is this the right place for this? Or name?
I think there is a similar Config object in rocketchip

I'll look for it.

I believe we can tear out all the TraceGen related stuff out of firechip. @zhemao do you still want it in here? TraceGenConfigs are now run out of Chipyard.

colinschmidt · 2020-03-16T22:54:43Z

generators/firechip/src/main/scala/TargetMixins.scala

+trait HasFireSimClockingImp extends HasAdditionalClocks {
+  val outer: HasTiles
+  val (tileClock, tileReset) = p(FireSimClockKey).additionalClocks.headOption match {
+    case Some(RationalClock(_, numer, denom)) if numer != denom => (clocks(1), ResetCatchAndSync(clocks(1), reset.toBool))


Does this check really do anything? I.e. are there generators that do this math and don't realize they are making a synchronized design?
Also are the magic numbers necessary?

The numer != denom check? No, but it saves the catch and sync hardware in the synchronous case. I can remove it.

I think how we plumb the clocks through the target is mostly a separate question to how we want to expose the generation of those clocks for firesim. I mostly just hacked together the prior to get something working quick.

In light of @jerryz123's recent push to harmonize the targets across firechip and chipyard, we should consider making our targets support multiclock by default. So for RTL bound for target-level RTL simulation or physical design, we'll need to define the other derived clocks somehow.

To remove the vector-indexing hack here i think what we want in the interim is to generate some clock-map [Key/String, ClockInfo] in the parameters object. We can provide some sugar to let the generator index the right element in the clock vector using the same key.

[ci skip]

colinschmidt

LGTM, pending a few questions.

colinschmidt · 2020-03-25T15:58:22Z

generators/firechip/src/main/scala/BridgeBinders.scala

          CompleteConfig(p(firesim.configs.MemModelKey), nastiKey, Some(AXI4EdgeSummary(edge))))
      })
    }).toSeq
  }
 })

 class WithTracerVBridge extends OverrideIOBinder({
-  (c, r, s, target: CanHaveTraceIOModuleImp) => target.traceIO.map(t => TracerVBridge(t)(target.p)).toSeq
+  (c, r, s, target: CanHaveTraceIOModuleImp) => target.traceIO match {
+    case Some(t) => t.traces.map(tileTrace => TracerVBridge(tileTrace)(target.p))


Why doesn't this bridge take a clock?

It does, it's included in the TileTraceIO as it is synchronous to the specific tile that produced it.

colinschmidt · 2020-03-25T16:15:04Z

generators/firechip/src/test/scala/ScalaTestSuite.scala

+  "FireSimQuadRocketMulticlockConfig",
+  "WithSynthAsserts_BaseF1Config")
+
+// Jerry broke these -- damn it Jerry.


Is this already tracked in a PR/Issue?

Oops
I don't think this tracegen-specific stuff should be tracked in Firechip anyways. Doing a tracegen simulation in firesim should just require changing the BuildTop key.

Yeah i'm in 100% agreement. No one runs the scala tests except me though. I'll fix it when i get the CI to run the scalatests for firechip.

[ci skip]

davidbiancolin added 12 commits November 1, 2019 17:17

[FireChip] Use clock in BridgeBinders

e3b30db

[firesim] Update TraceGen BridgeBinder

12485b8

[Firechip] Add support for Tile <-> Uncore rational division

bcddd6e

[firesim] Update the multiclock test

38834a9

[firechip] Commit some Eagle X-related mock configs

524299b

[firechip] Instantiate multiple TracerV bridges

3fbc074

[Firechip] Include reset in tracerv tokens

924f440

[TracerV] Drop the first token in comparison tests

b47e692

[Firechip] Make reverse instruction order in trace printf

e45c83f

WARNING: Point at a fork of boom @ davidbiancolin

43008ad

Merge remote-tracking branch 'origin/dev' into firesim-multiclock

d19ca81

[firesim] Update ClockBridge API

958332e

davidbiancolin added the DO NOT MERGE label Mar 13, 2020

davidbiancolin requested review from colinschmidt and abejgonzalez March 13, 2020 05:15

jerryz123 reviewed Mar 14, 2020

View reviewed changes

abejgonzalez reviewed Mar 16, 2020

View reviewed changes

abejgonzalez assigned davidbiancolin Mar 16, 2020

colinschmidt requested changes Mar 16, 2020

View reviewed changes

Merge remote-tracking branch 'origin/dev' into firesim-multiclock

d80c2f7

[ci skip]

davidbiancolin force-pushed the firesim-multiclock branch from 523f83d to d80c2f7 Compare March 18, 2020 16:22

davidbiancolin added 3 commits March 19, 2020 10:00

[firechip] Isolate all firesim-multiclock stuff in a single file

7a17323

Bump firesim for CI

1b71588

Bump FireSim

7704f38

colinschmidt approved these changes Mar 25, 2020

View reviewed changes

davidbiancolin added 2 commits March 25, 2020 10:20

Bump testchipip to dev

fbc47af

[ci skip]

Bump FireSim

b5b1587

davidbiancolin merged commit fe2f50f into dev Mar 25, 2020

alonamid removed the DO NOT MERGE label Apr 29, 2020

alonamid mentioned this pull request May 30, 2020

Chipyard 1.3 Release #500

Merged

davidbiancolin deleted the firesim-multiclock branch May 30, 2020 03:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Target-Facing Support for Multiclock Simulation in FireSim #468

Target-Facing Support for Multiclock Simulation in FireSim #468

davidbiancolin commented Mar 13, 2020

jerryz123 Mar 14, 2020

davidbiancolin Mar 15, 2020

abejgonzalez Mar 16, 2020 •

edited

Loading

abejgonzalez Mar 16, 2020

abejgonzalez Mar 16, 2020

abejgonzalez Mar 16, 2020

colinschmidt left a comment

colinschmidt Mar 16, 2020

davidbiancolin Mar 17, 2020

jerryz123 Mar 17, 2020

colinschmidt Mar 16, 2020

davidbiancolin Mar 17, 2020

colinschmidt left a comment

colinschmidt Mar 25, 2020

davidbiancolin Mar 25, 2020

colinschmidt Mar 25, 2020

jerryz123 Mar 25, 2020

davidbiancolin Mar 25, 2020

Target-Facing Support for Multiclock Simulation in FireSim #468

Target-Facing Support for Multiclock Simulation in FireSim #468

Conversation

davidbiancolin commented Mar 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abejgonzalez Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinschmidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinschmidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abejgonzalez Mar 16, 2020 •

edited

Loading