rocket-chip icon indicating copy to clipboard operation
rocket-chip copied to clipboard

BaseTile Auxiliary Constructors Bug

Open DecodeTheEncoded opened this issue 2 years ago • 5 comments

Inside the BaseTile there is val visibilityNode = p(TileVisibilityNodeKey) (it represents the view that the intra-tile masters have of the rest of the system). So, conceptually this diolomatic node should be in the scope of the Tile(Subclassing from the BaseTile---> RocketTile). However, the resulting location of this node is actually in the parent scope of the Tile, which is the tile_reset_domain(I change the Type from TLEphemeralNode to TLIdentityNode so that it appears on the graph): image The reason why this happens has to do with the BaseTile auxiliary constructor:

abstract class BaseTile private (val crossing: ClockCrossingType, q: Parameters)
    extends LazyModule()(q)
    with CrossesToOnlyOneClockDomain
    with HasNonDiplomaticTileParameters
    with HasLogicalTreeNode
{
  // Public constructor alters Parameters to supply some legacy compatibility keys
  def this(tileParams: TileParams, crossing: ClockCrossingType, lookup: LookupByHartIdImpl, p: Parameters) = {
    this(crossing, p.alterMap(Map(
      TileKey -> tileParams,
      TileVisibilityNodeKey -> { println("LazyScope shoot1 " + LazyModule.scope ); TLEphemeralNode()(ValName("tile_master"))},
      LookupByHartId -> lookup
    )))
    println("LazyScope shoot2 " + LazyModule.scope)
  }

Before executing the constructor of the parent class LazyModule()(q) (inside which the tile itself will be rightly pushed onto the LazyModule.scope stack ), the parameter q should be evaluated first, the q actually refers to :

p.alterMap(Map(
      TileKey -> tileParams,
      TileVisibilityNodeKey -> TLEphemeralNode()(ValName("tile_master")),
      LookupByHartId -> lookup
    ))

During the instantiation of TLEphemeralNode()(ValName("tile_master") scope of the node will be set (val scope: Option[LazyModule] = LazyModule.scope), however, the LazyModule.scope now is actually tile_reset_domain; Concequently the socpe of the pre instantiated TLEphemeralNode is the LazyModule.scope before the tile is pushed onto the stack, which is the resetDomain. This is just not right. I am actually not certain this is an actual bug or an intended behavior. Can you guys check this ? @hcook @sequencer

DecodeTheEncoded avatar May 12 '22 12:05 DecodeTheEncoded

@jerryhethatday In BaseTile, val visibilityNode = p(TileVisibilityNodeKey), visibilityNode will not be instantiated until TileVisibilityNodeKey is called. And TileVisibilityNodeKey is not passed until function "this" in BaseTile is called.

It can be found that we only call "this" when we build "RocketTile extend BaseTile(rocketParams, crossing, lookup, q)" in RocketTile.scala. For this reason, in RocketTile, defined val masterNode = visibilityNode and then masterNode :=* tlOtherMastersNode,otherwise we can use visibilityNode:=tlOtherMastersNode directly.

So the position of the visibilityNode you see in the graph is where the visibilityNode is actually instantiated, that is outside of BaseTile.

JACKLIAO0 avatar May 12 '22 15:05 JACKLIAO0

Welcome to Scala 2 inheritance antics. I highly encourage posting these sorts of questions on Stack Overflow using the rocket-chip tag since search engines can better index answers and time-to-time Scala folks can chip in an answer for Scala land questions like this one 😄

michael-etzkorn avatar May 12 '22 17:05 michael-etzkorn

I don’t this should be closed now. The question is

I am actually not certain this is an actual bug or an intended behavior. Can you guys check this ?

Basically the question is PRCI wrapper living in the RocketTile is a correct or not. Neither do I have a correct answer. This question may need to be answered by @hcook since he is the code owner of this part.

sequencer avatar May 12 '22 19:05 sequencer

I thought it was clear from @JACKLIAO0's answer how RocketTile's this is called on new TilePRCIDomain[TileType] explaining why the VisibilityNode appears where it does (even though it's ephemeral and not important to the graph anyway). Another way of thinking of it, the visibility node is used to bridge to other clients and should sit in the outermost portion of the tile. Either way, this isn't a bug and I closed it to encourage a repost to SO.

michael-etzkorn avatar May 12 '22 20:05 michael-etzkorn

See #2935, if we migrate to the "node generator" approach, the PRCI domain will live at where it was queried from.

sequencer avatar May 12 '22 20:05 sequencer