XiangShan icon indicating copy to clipboard operation
XiangShan copied to clipboard

Config: set minimal hartid width to 6

Open Tang-Haojin opened this issue 9 months ago • 4 comments

This can help users who only build one core but then manually instantiate more than two cores in the SoC.

Tang-Haojin avatar May 10 '24 09:05 Tang-Haojin

Thanks for doing this! Looks good to me.

We may also need a config to split XSTop and XSCore to change something outside of XSCore faster. Just leave it as future work.

cyyself avatar May 10 '24 10:05 cyyself

We may also need a config to split XSTop and XSCore to change something outside of XSCore faster. Just leave it as future work.

The major issue here is the diplomacy. Getting the correct parameters for outside XSCore requires all LazyModule nodes. We have to use fake nodes (and make sure they are the same as XSCore) if we want to separate XSCore and uncore.

Actually we have a transform to copy Core 0 to other cores. This reduces the FIRRTL -> verilog time but does not help the Chisel elaboration stage.

We don't know how to fix the issue better. cc @sequencer

poemonsense avatar May 10 '24 13:05 poemonsense

We may also need a config to split XSTop and XSCore to change something outside of XSCore faster. Just leave it as future work.

The major issue here is the diplomacy. Getting the correct parameters for outside XSCore requires all LazyModule nodes. We have to use fake nodes (and make sure they are the same as XSCore) if we want to separate XSCore and uncore.

Indeed.

Actually we have a transform to copy Core 0 to other cores. This reduces the FIRRTL -> verilog time but does not help the Chisel elaboration stage.

Another way is to use LazyModule cloning, but we need to fix ChiselDB and ConstantIn first.

cyyself avatar May 10 '24 13:05 cyyself

but we need to fix ChiselDB and ConstantIn first.

Yes. No matter how we address the multicore issue, we need to ensure they are the same. ChiselDB and ConstantIn are destroying the dedup between multiple cores.

poemonsense avatar May 10 '24 13:05 poemonsense

We don't know how to fix the issue better. cc @sequencer

Please use D/I... I'm prototyping the new diplomacy framework, but that needs a large refactor...

sequencer avatar May 11 '24 05:05 sequencer

This can help users who only build one core but then manually instantiate more than two cores in the SoC.

Please wait, this line in cpl2 should also be fixed https://github.com/OpenXiangShan/CoupledL2/pull/102/files#diff-566b098f719cfe2cdea01bb41cd67c977a43516affa844e0512e321dd742170bR83

cyyself avatar May 11 '24 06:05 cyyself

From my mind:

  • All LazyModule should only be a SerializableModule that only depends on a SerializableModuleParameter. In this case, all SerializableModule can be safely instantiated via D/I. and is cacheable in Chisel.
  • All LazyModule should be a FixedIOModule that can instantiate its Interface(IO) before the Module elaboration.

The current implementation in diplomacy has these design warts, which won't be supported:

  • CDE.
  • implicit parameters.
  • In-memory linking from Edge to Bundle.

I'm drafting a NoC-based SoC framework w/ this methodology, eta 6m to get it ready.

sequencer avatar May 11 '24 06:05 sequencer

I found a bug since the last time I modified CoupledL2:

src/main/scala/xiangshan/L2Top.scala:91

  val l2cache = coreParams.L2CacheParamsOpt.map(l2param =>
    LazyModule(new CoupledL2()(new Config((_, _, _) => {
      case L2ParamKey => l2param.copy(
          hartIds = Seq(p(XSCoreParamsKey).HartId),

Then, l2cache will only get a hartId sized 1.

But we use the size of this List to construct hartId in CoupledL2:

coupledL2/src/main/scala/coupledL2/CoupledL2.scala:84

  lazy val msgSizeBits = edgeIn.bundle.sizeBits
  lazy val sourceIdAll = 1 << sourceIdBits

  lazy val hartIdLen: Int = log2Up(cacheParams.hartIds.length)

  val mshrsAll = cacheParams.mshrs

So, currently, the hartIdLen in coupledL2 is wrong.

I wanted to fix it today, but then I found many things that should be fixed first, especially for ConstantIn and ChiselDB support for hartid from io. If you want to use it temporarily, you can just apply the following patches with your branch:

Xiangshan:

diff --git a/src/main/scala/top/Top.scala b/src/main/scala/top/Top.scala
index 11b5b8806..cab8ad853 100644
--- a/src/main/scala/top/Top.scala
+++ b/src/main/scala/top/Top.scala
@@ -27,6 +27,7 @@ import device._
 import chisel3.stage.ChiselGeneratorAnnotation
 import org.chipsalliance.cde.config._
 import freechips.rocketchip.diplomacy._
+import freechips.rocketchip.tile._
 import freechips.rocketchip.tilelink._
 import freechips.rocketchip.jtag.JTAGIO
 import chisel3.experimental.{annotate, ChiselAnnotation}
@@ -74,6 +75,7 @@ class XSTop()(implicit p: Parameters) extends BaseXSSoc() with HasSoCParameter
         hartIds = tiles.map(_.HartId),
         FPGAPlatform = debugOpts.FPGAPlatform
       )
+      case MaxHartIdBits => p(MaxHartIdBits)
     })))
   )
 
diff --git a/src/main/scala/xiangshan/L2Top.scala b/src/main/scala/xiangshan/L2Top.scala
index 7102237d3..182b87c8f 100644
--- a/src/main/scala/xiangshan/L2Top.scala
+++ b/src/main/scala/xiangshan/L2Top.scala
@@ -21,7 +21,7 @@ import org.chipsalliance.cde.config._
 import chisel3.util.{Valid, ValidIO}
 import freechips.rocketchip.diplomacy._
 import freechips.rocketchip.interrupts._
-import freechips.rocketchip.tile.{BusErrorUnit, BusErrorUnitParams, BusErrors}
+import freechips.rocketchip.tile.{BusErrorUnit, BusErrorUnitParams, BusErrors, MaxHartIdBits}
 import freechips.rocketchip.tilelink._
 import coupledL2.{CoupledL2, L2ParamKey}
 import system.HasSoCParameter
@@ -94,6 +94,7 @@ class L2Top()(implicit p: Parameters) extends LazyModule
           hartIds = Seq(p(XSCoreParamsKey).HartId),
           FPGAPlatform = debugOpts.FPGAPlatform
         )
+      case MaxHartIdBits => p(MaxHartIdBits)
     })))
   )
   val l2_binder = coreParams.L2CacheParamsOpt.map(_ => BankBinder(coreParams.L2NBanks, 64))

submodule CoupledL2

diff --git a/src/main/scala/coupledL2/CoupledL2.scala b/src/main/scala/coupledL2/CoupledL2.scala
index e6cf2ae..4e743f9 100644
--- a/src/main/scala/coupledL2/CoupledL2.scala
+++ b/src/main/scala/coupledL2/CoupledL2.scala
@@ -23,6 +23,7 @@ import chisel3._
 import chisel3.util._
 import utility.{FastArbiter, ParallelMax, ParallelPriorityMux, Pipeline, RegNextN}
 import freechips.rocketchip.diplomacy._
+import freechips.rocketchip.tile.MaxHartIdBits
 import freechips.rocketchip.tilelink._
 import freechips.rocketchip.tilelink.TLMessages._
 import freechips.rocketchip.util._
@@ -83,7 +84,7 @@ trait HasCoupledL2Parameters {
   lazy val msgSizeBits = edgeIn.bundle.sizeBits
   lazy val sourceIdAll = 1 << sourceIdBits
 
-  lazy val hartIdLen: Int = log2Up(cacheParams.hartIds.length)
+  lazy val hartIdLen: Int = p(MaxHartIdBits)
 
   val mshrsAll = cacheParams.mshrs
   val idsAll = 256// ids of L2 //TODO: Paramterize like this: max(mshrsAll * 2, sourceIdAll * 2)

submodule HuanCun

diff --git a/src/main/scala/huancun/HuanCun.scala b/src/main/scala/huancun/HuanCun.scala
index 5722a71..52f9eb9 100644
--- a/src/main/scala/huancun/HuanCun.scala
+++ b/src/main/scala/huancun/HuanCun.scala
@@ -23,6 +23,7 @@ import org.chipsalliance.cde.config.Parameters
 import chisel3._
 import chisel3.util._
 import freechips.rocketchip.diplomacy._
+import freechips.rocketchip.tile.MaxHartIdBits
 import freechips.rocketchip.tilelink._
 import freechips.rocketchip.tilelink.TLMessages._
 import freechips.rocketchip.util.{BundleField, BundleFieldBase, UIntToOH1}
@@ -103,7 +104,7 @@ trait HasHuanCunParameters {
 
   lazy val outerSinkBits = edgeOut.bundle.sinkBits
 
-  lazy val hartIdLen: Int = log2Up(cacheParams.hartIds.length)
+  lazy val hartIdLen: Int = p(MaxHartIdBits)
 
   val block_granularity = if (!cacheParams.inclusive && cacheParams.clientCaches.nonEmpty) {
     cacheParams.clientCaches.head.blockGranularity

cyyself avatar May 11 '24 08:05 cyyself

I found many things that should be fixed first, especially for ConstantIn and ChiselDB support for hartid from io.

FYI, https://github.com/OpenXiangShan/XiangShan/pull/2672

poemonsense avatar May 11 '24 08:05 poemonsense

Also, by default, both ChiselDB and constantin are disabled. We can skip them first and fix them in the next step.

poemonsense avatar May 11 '24 08:05 poemonsense

Also, by default, both ChiselDB and constantin are disabled. We can skip them first and fix them in the next step.

There is an AlwaysBasicDB which mainly used by tllog. It may make dedup fail.

Tang-Haojin avatar May 11 '24 08:05 Tang-Haojin

[Generated by IPC robot] commit: f4b08c240986b6eb3a1c9e9bc06da4dd8cd75e3d

commit astar copy_and_run coremark gcc gromacs lbm linux mcf microbench milc namd povray wrf xalancbmk
f4b08c2 1.854 0.449 2.103 1.182 2.170 2.181 2.329 0.973 1.386 1.294 2.742 2.605 2.283 2.951

master branch:

commit astar copy_and_run coremark gcc gromacs lbm linux mcf microbench milc namd povray wrf xalancbmk
1e018fb 1.854 0.450 2.103 1.190 2.468 2.593 2.330 0.962 1.377 1.427 3.123 2.639 2.451 2.960
3c71816 1.854 0.450 2.103 1.190 2.468 2.593 2.330 0.962 1.377 1.427 3.123 2.639 2.451 2.960
dfe034b 1.854 0.449 2.103 1.182 2.170 2.181 2.328 0.972 1.386 1.294 2.742 2.592 2.283 2.951
bdc1606 1.854 0.449 2.101 1.182 2.170 2.181 2.329 0.973 1.386 1.294 2.742 2.589 2.283 2.951
dc5a918 1.854 0.449 2.103 1.181 2.170 2.181 2.329 0.973 1.386 1.294 2.742 2.589 2.283 2.951
bad6084 1.854 0.449 2.103 1.182 2.170 2.181 2.329 0.972 1.386 1.294 2.742 2.589 2.283 2.951
c686adc 1.854 0.449 2.103 1.182 2.170 2.181 2.335 0.972 1.386 1.294 2.742 2.589 2.283 2.951
bc3d558 1.854 0.449 2.100 1.182 2.170 2.181 2.329 0.972 1.380 1.294 2.742 2.589 2.283 2.951
a58f171 1.854 0.449 2.104 1.182 2.170 2.181 2.329 0.972 1.386 1.294 2.742 2.586 2.283 2.951
ff74867 1.898 0.448 2.105 1.186 2.173 2.175 2.335 0.960 1.372 1.288 2.745 2.583 2.285 2.958

XiangShanRobot avatar May 12 '24 18:05 XiangShanRobot

[Generated by IPC robot] commit: 083769034d825678d17e79be1594602be0700ea8

commit astar copy_and_run coremark gcc gromacs lbm linux mcf microbench milc namd povray wrf xalancbmk
0837690 1.854 0.450 2.103 1.190 2.468 2.593 2.330 0.962 1.377 1.427 3.123 2.639 2.451 2.960

master branch:

commit astar copy_and_run coremark gcc gromacs lbm linux mcf microbench milc namd povray wrf xalancbmk
5e237ba
363530d
a72b131
05d833a 1.854 0.450 2.103 1.190 2.468 2.593 2.330 0.961 1.377 1.427 3.123 2.639 2.451 2.960
9cb05b4 0.450 2.103 2.330 1.377 2.639
4daa5bf 0.450 2.103 1.190 2.468 2.593 2.329 0.962 1.377 1.427 3.123 2.639 2.960
1e018fb 1.854 0.450 2.103 1.190 2.468 2.593 2.330 0.962 1.377 1.427 3.123 2.639 2.451 2.960
3c71816 1.854 0.450 2.103 1.190 2.468 2.593 2.330 0.962 1.377 1.427 3.123 2.639 2.451 2.960
dfe034b 1.854 0.449 2.103 1.182 2.170 2.181 2.328 0.972 1.386 1.294 2.742 2.592 2.283 2.951
bdc1606 1.854 0.449 2.101 1.182 2.170 2.181 2.329 0.973 1.386 1.294 2.742 2.589 2.283 2.951

XiangShanRobot avatar May 14 '24 03:05 XiangShanRobot