Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attach TLError LogicalTreeNodes to subsystem LogicalTreeNode #2410

Merged
merged 10 commits into from
Apr 16, 2020
Merged

Attach TLError LogicalTreeNodes to subsystem LogicalTreeNode #2410

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
595e0e9
Create OMErrorDevice.scala
mwachs5 Mar 18, 2020
0e08326
TLError: implement HasLogicalTreeNode
albertchen-sifive Apr 11, 2020
8ad2f8a
OMErrorDevice: remove specifications
mwachs5 Apr 11, 2020
f5db4b4
Add Object model for Zero device
mwachs5 Apr 11, 2020
86ba877
TLZero: add OM calls
mwachs5 Apr 11, 2020
bf52274
Merge pull request #2411 from chipsalliance/attach-TLError-logicalTre…
albertchen-sifive Apr 12, 2020
ba744ca
BaseSubsystem: move LogicalTree add out of lazy val
albertchen-sifive Apr 12, 2020
5b6f37c
BaseSubsystem: add TLZero LogicalTreeNodes
albertchen-sifive Apr 13, 2020
1910582
CanHaveBuiltInDevices: move attach fn to object
albertchen-sifive Apr 13, 2020
042c8af
update OM names and descriptions
albertchen-sifive Apr 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/main/scala/devices/tilelink/CanHaveBuiltInDevices.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,30 @@ trait HasBuiltInDeviceParams {
val errorDevice: Option[DevNullParams]
}

sealed trait BuiltInDevices {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def errorOpt: Option[TLError]
def zeroOpt: Option[TLZero]
}

/* Optionally add some built-in devices to a bus wrapper */
trait CanHaveBuiltInDevices { this: TLBusWrapper =>

def attachBuiltInDevices(params: HasBuiltInDeviceParams) {
params.errorDevice.foreach { dnp => LazyScope("wrapped_error_device") {
def attachBuiltInDevices(params: HasBuiltInDeviceParams): BuiltInDevices = new BuiltInDevices {
val errorOpt = params.errorDevice.map { dnp => LazyScope("wrapped_error_device") {
val error = LazyModule(new TLError(
params = dnp,
beatBytes = beatBytes))

error.node := TLBuffer() := outwardNode
error
}}

params.zeroDevice.foreach { addr => LazyScope("wrapped_zero_device") {
val zeroOpt = params.zeroDevice.map { addr => LazyScope("wrapped_zero_device") {
val zero = LazyModule(new TLZero(
address = addr,
beatBytes = beatBytes))
zero.node := TLFragmenter(beatBytes, blockBytes) := TLBuffer() := outwardNode
zero
}}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/devices/tilelink/DevNull.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ case class DevNullParams(
* They may discard writes, refuse to respond to requests, issue error responses,
* or otherwise violate 'expected' memory behavior.
*/
abstract class DevNullDevice(params: DevNullParams, minLatency: Int, beatBytes: Int, device: SimpleDevice)
abstract class DevNullDevice(params: DevNullParams, minLatency: Int, beatBytes: Int, protected val device: SimpleDevice)
(implicit p: Parameters)
extends LazyModule with HasClockDomainCrossing {
val xfer = if (params.maxTransfer > 0) TransferSizes(1, params.maxTransfer) else TransferSizes.none
Expand Down
18 changes: 18 additions & 0 deletions src/main/scala/devices/tilelink/Error.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,30 @@ import freechips.rocketchip.tilelink._
import freechips.rocketchip.util._
import scala.math.min

import freechips.rocketchip.diplomaticobjectmodel.{DiplomaticObjectModelAddressing, HasLogicalTreeNode}
import freechips.rocketchip.diplomaticobjectmodel.logicaltree.LogicalTreeNode
import freechips.rocketchip.diplomaticobjectmodel.model.{OMErrorDevice, OMComponent}

/** Adds a /dev/null slave that generates TL error response messages */
class TLError(params: DevNullParams, buffer: Boolean = true, beatBytes: Int = 4)(implicit p: Parameters)
extends DevNullDevice(params,
minLatency = if (buffer) 1 else 0,
beatBytes, new SimpleDevice("error-device", Seq("sifive,error0")))
with HasLogicalTreeNode
{
lazy val logicalTreeNode: LogicalTreeNode = new LogicalTreeNode(() => Some(device)) {
def getOMComponents(resourceBindings: ResourceBindings, children: Seq[OMComponent] = Nil) = {
val Description(name, mapping) = device.describe(resourceBindings)
val memRegions = DiplomaticObjectModelAddressing.getOMMemoryRegions(name, resourceBindings, None)
val interrupts = DiplomaticObjectModelAddressing.describeInterrupts(name, resourceBindings)
Seq(OMErrorDevice(
memoryRegions = memRegions,
interrupts = interrupts,
specifications = Nil
))
}
}

lazy val module = new LazyModuleImp(this) {
import TLMessages._
import TLPermissions._
Expand Down
11 changes: 11 additions & 0 deletions src/main/scala/diplomaticobjectmodel/model/OMErrorDevice.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// See LICENSE.SiFive for license details.

package freechips.rocketchip.diplomaticobjectmodel.model


case class OMErrorDevice(
memoryRegions: Seq[OMMemoryRegion],
interrupts: Seq[OMInterrupt],
specifications: Seq[OMSpecification],
mwachs5 marked this conversation as resolved.
Show resolved Hide resolved
_types: Seq[String] = Seq("OMErrorDevice", "OMDevice", "OMComponent", "OMCompoundType")
) extends OMDevice
16 changes: 15 additions & 1 deletion src/main/scala/subsystem/BaseSubsystem.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,21 @@ abstract class BaseSubsystem(implicit p: Parameters) extends BareSubsystem
}
}

lazy val logicalTreeNode = new SubsystemLogicalTreeNode()
lazy val logicalTreeNode = {
val subsystemLogicalTreeNode = new SubsystemLogicalTreeNode()
val builtInDevices = Seq(
pbus.builtInDevices,
fbus.builtInDevices,
mbus.builtInDevices,
cbus.builtInDevices
mwachs5 marked this conversation as resolved.
Show resolved Hide resolved
)
for (builtIn <- builtInDevices) {
builtIn.errorOpt.foreach { error =>
mwachs5 marked this conversation as resolved.
Show resolved Hide resolved
LogicalModuleTree.add(subsystemLogicalTreeNode, error.logicalTreeNode)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get how lazy vals work

subsystemLogicalTreeNode
}
}


Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/subsystem/FrontBus.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ class FrontBus(params: FrontBusParams)(implicit p: Parameters)
with CanHaveBuiltInDevices
with CanAttachTLMasters
with HasTLXbarPhy {
attachBuiltInDevices(params)
val builtInDevices = attachBuiltInDevices(params)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
2 changes: 1 addition & 1 deletion src/main/scala/subsystem/MemoryBus.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class MemoryBus(params: MemoryBusParams)(implicit p: Parameters)
if (params.replicatorMask == 0) xbar.node else { xbar.node :=* RegionReplicator(params.replicatorMask) }
def outwardNode: TLOutwardNode = ProbePicker() :*= xbar.node
def busView: TLEdge = xbar.node.edges.in.head
attachBuiltInDevices(params)
val builtInDevices = attachBuiltInDevices(params)

def toDRAMController[D,U,E,B <: Data]
(name: Option[String] = None, buffer: BufferParams = BufferParams.none)
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/subsystem/PeripheryBus.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class PeripheryBus(params: PeripheryBusParams, name: String)(implicit p: Paramet
def outwardNode: TLOutwardNode = node
def busView: TLEdge = fixer.node.edges.in.head

attachBuiltInDevices(params)
val builtInDevices = attachBuiltInDevices(params)

def toTile
(name: Option[String] = None, buffer: BufferParams = BufferParams.none)
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/subsystem/SystemBus.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class SystemBus(params: SystemBusParams)(implicit p: Parameters)
def outwardNode: TLOutwardNode = system_bus_xbar.node
def busView: TLEdge = system_bus_xbar.node.edges.in.head

attachBuiltInDevices(params)
val builtInDevices = attachBuiltInDevices(params)

def fromTile
(name: Option[String], buffer: BufferParams = BufferParams.none, cork: Option[Boolean] = None)
Expand Down