Attach TLError LogicalTreeNodes to subsystem LogicalTreeNode

[albertchen-sifive]

This is my attempt at adding OMErrorDevice to the object model. It seems to work based on visual inspection of the resulting objectModel.json. Looking for feedback on whether this is the best way of plumbing and attaching the logicalTreeNodes.

Related issue:
includes #2343

Type of change: other enhancement

Impact: API modification

• factor out CanHaveBuiltInDevices.attachBuiltInDevices into attach helper method in BuiltInDevices companion object
• CanHaveBuiltInDevices has abstract field def builtInDevices: BuiltInDevices
• TLError extends HasLogicalTreeNode
• TLZero extends HasLogicalTreeNode
• argument device of DevNullDevice is a protected val

Development Phase: implementation

Release Notes

Add Object Model for TLError Device.

[richardxia]

This more of a question for me to learn more about Scala than about whether this is the right or wrong thing to do.

Since this is only used in one place, in an anonymous class instance, what's the practical difference between defining this as a sealed trait + anonymous class instance vs. defining a (final) case class and assigning the members normally? I think I can appreciate this technique being used for non-sealed traits, but the fact that this is sealed is interesting to me.

[albertchen-sifive]

I the main difference I care about vs a final case class is that the case class generates a bunch of methods and extends stuff that I don't think are appropriate/useful. like case class extends serializable which TLError and TLZero definitely are not and it generates stuff like unapply that I don't want exposed because that API will probably be broken.

sealed is for similar reasons. to prevent anyone else from creating BuiltInDevices outside of attachBuiltInDevices and just narrow down the API surface area.

just my personal preference, maybe I'm being a too cautious or this isn't the right approach? open to changing this.

[richardxia]

just my personal preference, maybe I'm being a too cautious or this isn't the right approach? open to changing this.

No, I think it's a good idea to limit API surface area. I was mostly wondering sealed trait vs final case class and not sealed trait vs (not sealed) trait. I hadn't thought of the extra methods and traits that case classes provide as being extra stuff that consumers could possibly depend on that would become later liabilities, but now that you point it out, I agree, especially because this data structure is, at the moment, an arbitrary bundle of "stuff", and we may want to reserve the right to change this in the future.

[richardxia]

Not sure if this is more readable, but how about:

Suggested change

	for (builtIn <- builtInDevices) {
	builtIn.errorOpt.foreach { error =>
	LogicalModuleTree.add(subsystemLogicalTreeNode, error.logicalTreeNode)
	}
	}
	for (builtIn <- builtInDevices; error <- builtIn.errorOpt) {
	LogicalModuleTree.add(subsystemLogicalTreeNode, error.logicalTreeNode)
	}

?

[albertchen-sifive]

looking at this again, I'm not sure why I didn't just do this to be consistent

Suggested change

	for (builtIn <- builtInDevices) {
	builtIn.errorOpt.foreach { error =>
	LogicalModuleTree.add(subsystemLogicalTreeNode, error.logicalTreeNode)
	}
	}
	builtInDevices.foreach { builtIn =>
	builtIn.errorOpt.foreach { error =>
	LogicalModuleTree.add(subsystemLogicalTreeNode, error.logicalTreeNode)
	}
	}

but I dunno. I guess the your version looks the best to me.

[mwachs5]

I think @albertchen-sifive 's is more what I'm used to in the code base, but either change looks good and symmetrical :)

[albertchen-sifive]

more importantly. I just realized that we are overriding this field. so I guess this stuff should go in an attachBuiltInDevicesLogicalTreeNodes method that can be called in the constructor of the LogicalTreeNode?

[richardxia]

Ah, I had forgotten about the lazy val being overridden by subclasses. Now that you've brought it up, I don't actually think we want any of this stuff happening in the constructor of the LogicalTreeNode. Is there a problem with just moving all of the LogicalModuleTree.add() statements below/outside of the lazy val logicalTreeNode = { } statement?

[albertchen-sifive]

that would force evaluation of the lazy val logicalTreeNode early? doesn't seem like it matters when the logicalTreeNode gets evaluated though, because it still works if I move it outside of the block.

[richardxia]

Yeah, that should be fine, since we have a pattern of doing this in a number of places. The logicalTreeNodes are only really meant to create the tree of nodes without fulling evaluated the nodes, somewhat analogous to Diplomatic nodes.

[albertchen-sifive]

I don't get how lazy vals work

[mwachs5]

Curious if this is going to massively conflict with @hcook's #2327... if so would be nice to get this in first...

[mwachs5]

is there a reason this val can't go into the trait CanHaveBuiltInDevices? Maybe as a def or lazy val?

[albertchen-sifive]

this just seemed like the least intrusive way of adding it. the val can go in CanHaveBuiltInDevices, but then it would need to have access to a params: HasBuiltInDeviceParams. and I wasn't sure if attachBuiltInDevices was actually being called everywhere that CanHaveBuiltInDevices was being mixed in.

but it does look like the attach method is unconditionally called everywhere that the trait is mixed in. and the devices are already optionally attached based on the params. so yeah, it makes sense to me to have the val in CanHaveBuiltInDevices and just inline the attachBuiltInDevices method.

[albertchen-sifive]

but that seems like a cake-pattern and we're trying to avoid that?

[richardxia]

but that seems like a cake-pattern and we're trying to avoid that?

We should all be clear on why we are (or are not?) avoiding the cake pattern in future code, since it's rarely the case that we would conclude "X is always bad and we should never do X again". Not having a shared understanding of what the root problems actually are could lead to bandwagoning and scapegoating where we dump a solution without knowing why and end up building a new solution that runs into similar problems. My impression of why the way we have used the cake pattern has been problematic is because:

• We end up creating these god classes which have to know about every possible trait that we may want to use, causing every addition of a new trait or a change to a trait to have to propagate up to the god class. The reason we have the "CanHaveX" pattern is because we've turned the "might have or might not have feature X" into an optional thing everywhere.
• Parameterizing the concrete instances of the cake interfaces becomes challenging due to statement ordering issues and restrictions on constructors. Coupled with the fact that the Chisel DSL essentially requires people to write HDL code directly in the "constructor" of a Scala class, this makes it difficult to provide a consistent interface for concrete implementations of the cake interface.

See https://kubuszok.com/2018/cake-antipattern/ for a detailed blog post on the Scala cake pattern and why the author at least considers it to be an antipattern. See specifically "Adding dependencies", which provides a concrete example of a god class, and "Initialization order", which describes the ordering and initialization issues.

With these points in mind, my personal response would be that to best avoid the two problems above without also making a huge architectural change to the existing code would be to decouple the interface from the implementation. We should not let the amount of typing required dictate what the interface should be. If we think that if a module says that it "can have built-in devices" should imply that we can retrieve said built-in devices under a uniform interface, then I think it is a good idea to define a def. However, if we think that different modules may want to expose them in different ways, then we should hold off from defining an interface.

[mwachs5]

@richardxia I'm not sure what you are concretely suggesting, or what you need to know to make a concrete suggestion

[richardxia]

I need to know what a CanHaveBuiltInDevices conceptually represents, and I need to know how we intend for concrete implementations to use it as well as how consumers of the concrete implementations are meant to use it. Otherwise, I don't think it's possible to have a conversation about whether we should pull the val builtInDevices = attachBuiltInDevices(params) into the trait definition.

Let me flip this onto you: Is there a reason this val should go into the trait CanHaveBuiltInDevices?

[albertchen-sifive]

I don't actually know what a BuiltInDevice is or the naming-conventions of these traits, but here's what I think:

I would think that CanHaveBuiltInDevices would have some field builtInDevices: BuiltInDevices so that consumers of the interface can know about and get some reference to the attached Option[TLError/TLZero]. right now it just defines a method attachBuiltInDevices(params) that can optionally be called to optionally attach devices based on params. If CanHaveBuiltInDevices doesn't define some interface for getting at the attached built-in devices, I don't quite understand the purpose of that trait. There's no reason attachBuiltInDevices needs to be implemented as a method inside a trait with self-type vs a regular function with a few extra arguments. and I don't think the attachBuiltInDevices method is helpful to consumers of the interface because if you call it from outside you risk instantiating and attaching devices multiple times, plus you'll also need a reference to the original LazyScope if you want the attach to actually happen in the same module that mixed in CanHaveBuiltInDevices.

I propose having an abstract def builtInDevices: BuiltInDevices in CanHaveBuiltInDevices, getting rid of the TLBusWrapper self-type, and adding a attachBuiltInDevices(params: HasBuiltInParams with HasTLBusParams, outwardNode: TLOutwardNode) to the BuiltInDevices companion object. This way we can avoid adding another lazy val to the cake and still provide a useful interface for consumers.

[richardxia]

That sounds like a reasonable proposal to me, and I agree with the trait otherwise being useless since the attach method could just be in a standalone function and also that it is dangerous to call it twice.

[mwachs5]

Still interested in @hcook feedback

[albertchen-sifive]

I just realized that https://github.com/chipsalliance/rocket-chip/blob/master/src/main/scala/subsystem/MemoryBus.scala#L60 is a def rather than a val and now it only get's called once since outwardNode is an argument whereas before outwardNode was a method that was called twice within attachBuiltInDevices(params). so I think I'm going to change that argument to => TLOutwardNode. or should I just change it to busWrapper: TLBusWrapper and call busWrapper.outwardNode?

[hcook]

@albertchen-sifive I'm not really sure why they are defs anymore; I think it is a historical artifact and not a requirement. In my bus refactoring PR I actually changed the code like so, with no apparent ill-effect. Perhaps you could adopt this code in order to reduce my eventual merge conflict:

  private def replicate(node: TLInwardNode): TLInwardNode =
    if (params.replicatorMask == 0) node else { node :*=* RegionReplicator(params.replicatorMask) }
  val inwardNode: TLInwardNode = replicate(xbar.node)
  val outwardNode: TLOutwardNode = ProbePicker() :*= xbar.node

That said, I also have no objection to => TLOutwardNode either, in order to retain maximum generality.

[albertchen-sifive]

Perhaps you could adopt this code in order to reduce my eventual merge conflict:

I think it might be better if I leave it as is? I haven't changed those lines so they shouldn't conflict with your change, and everything should still work with or without that change. and there's a good chance that I'll copy past that snippet incorrectly and cause a conflict.