Adding functionality to check whether current packets are diplomatically valid for both master and slave #2505

brrmorre · 2020-06-05T21:56:43Z

Related issue:
Adding functionally to check whether current packets are diplomatically valid for both master and slave instead of just checking what the slave supports. We now want to check for both what the master can emit and what the slave can support. We want to check for what is expected on the tilelink edge using both information about what the master can emit and what the slave can support or vice versa.

Type of change: feature request

Impact: API modification

Development Phase: implementation

Release Notes

…ed function calls in monitors

jchang0 · 2020-06-09T00:19:07Z

src/main/scala/tilelink/Parameters.scala

    }
  }

+  private def emitHelper(


Don't insert this code between the definition of safety_helper and usage of safety_helper, because it seems like the safety_helper is only used here.

jchang0 · 2020-06-09T00:22:03Z

src/main/scala/tilelink/Parameters.scala

+    sourceId: UInt,
+    lgSize:  UInt,
+    range:   Option[TransferSizes]): Bool = {
+    def trim(x: TransferSizes) = range.map(_.intersect(x)).getOrElse(x)


why do we need trim?

It seems asymmetric to "ignore" the range argument that is passed into the expectsXYX functions below. The helper that helps with sourceId-based calculations and the helper that helps with address-based calculations should probably treat range in the same way?

We agreed that we do not need to be concerned with range trimming for the version of these functions used for VIP circuitry.

hcook · 2020-06-09T00:53:02Z

src/main/scala/tilelink/Parameters.scala

+
+  //We don't use Safe vs Fast because we don't have the equivalent of address decoder for sourceIds
+// TODO (can use sourceId here to see if the specific master can support this transaction)
+  def expectsProbeSlaveToMaster      = master.supportsProbe


Duality: these B channel messages from SlaveToMaster are:

being routed to a master based on sourceId bits, so they need to be passed to a helper that checks whether the master that owns certain sourceIds claimed to support transactions of this size/type

being sent from a slave owning certain addresses, so they need to be passed to a helper that checks with the slave that owns certain addresses claimed to emit transactions of this size/type

hcook · 2020-06-09T00:55:21Z

src/main/scala/tilelink/Parameters.scala

+
+  // For emits, check that the source is allowed to send this transactions
+// TODO emitAcquire, source => sourceId (or don't need sourceId at all)
+//      Probe, source => address


These A channel messages from MasterToSlave are:

being routed to a slave based on address bits, so they need to be passed to a helper that checks whether the slave that owns certain addresses claimed to support transactions of this size/type

being sent from a master using sourceId bits, so they need to be passed to a helper that checks whether the master that owns certain sourceIds claimed to emit transactions of this size/type

hcook · 2020-06-09T01:06:32Z

src/main/scala/tilelink/Parameters.scala

-    if (allSame) member(clients(0)).containsLg(lgSize) else {
-      Mux1H(find(id), clients.map(member(_).containsLg(lgSize)))
+  // Available during RTL runtime, checks to see if (id, size) is supported by the master's (master's)  diplomatic parameters
+  private def safety_helper(member: TLMasterParameters => TransferSizes)(id: UInt, lgSize: UInt) = {


We need to reason about which features of the proposedemitHelper need to be merged with this function and whether they actually produce different logic in valuable ways.

I recommend changing the name of what is currently supportsHelper to be addressHelper and
the combination of safety_helper +emitsHelper to be sourceIdHelper, since I believe the supports/emits aspect is orthogonal (e.g. it is just whichever TLMasterParameters => TransferSizes you happen to pass in to sourceIdHelper, which could be either about supporting or about emitting).

Put another way, what really matters to the circuit generation helpers is which bit fields of the message you are filtering on, and since there are two options (id vs address), there should be two helper functions, one defined on TLMasterPortParameters (ids) and one defined on TLSlavePortParameters (addresses)

hcook · 2020-06-09T01:15:01Z

src/main/scala/tilelink/Parameters.scala

+  // Available during RTL runtime, checks to see if (id, size) is supported by the master's (master's)  diplomatic parameters
+  private def safety_helper(member: TLMasterParameters => TransferSizes)(id: UInt, lgSize: UInt) = {
+    val allSame = masters.map(member(_) == member(masters(0))).reduce(_ && _)
+    if (allSame) member(masters(0)).containsLg(lgSize) else {


I think this if statement is a coarse generalization of the groupBy in the emitsHelper version; the case where there is only one group.

src/main/scala/tilelink/Parameters.scala

hcook · 2020-06-09T01:18:32Z

src/main/scala/tilelink/Parameters.scala

@@ -534,20 +537,53 @@ class TLSlavePortParameters private(
  // Does this Port manage this ID/address?
  def containsSafe(address: UInt) = findSafe(address).reduce(_ || _)

+  // all we need to change about this is adding 2 members, one for the tlmaster parametrs and one for the tlslave parameters
+  // All wee need to do is change the membership function to instead of only slave pamaters it looks at the intersection of both master and slave
+  // do we need to pass in two different arguments


I think we concluded below that two different helper functions are necessary and that we will && the results of the duals together in the edge function. So this one should remain focused on TLSlaveParameters => TransferSizes cases that look at address.

jchang0 · 2020-06-12T22:35:32Z

src/main/scala/tilelink/Parameters.scala

-    val allSame = clients.map(member(_) == member(clients(0))).reduce(_ && _)
-    if (allSame) member(clients(0)).containsLg(lgSize) else {
-      Mux1H(find(id), clients.map(member(_).containsLg(lgSize)))
+  // Available during RTL runtime, checks to see if (id, size) is supported by the master's (master's)  diplomatic parameters


terpstra · 2020-06-15T22:14:53Z

As a follow-up PR we might want to split fast/safe into:

a VIP checker API that consults both the full address (not assumed to be legal) and the full source (not assumed to be legal) -- range is not relevant
a what possible sizes of Get might flow over this edge API (that does not look at address/source; just an intersection of the unions of master with the unions of slaves, but does include an optional range); Place-holder name: isMessagePossible
a check for whether or not a message (+size) is legal for a given address (assumed to be legal and given a range within which the checked size will fall) -- used by masters; Place-holder name: canMasterSend
a check for whether or not a message (+size) is legal for a given source (assumed to be legal and given a range within which the checked size will fall) -- used by slaves; Place-holder name: canSlaveSend

src/main/scala/tilelink/Parameters.scala

hcook · 2020-06-15T22:37:34Z

src/main/scala/tilelink/Parameters.scala

+  val supportsPutPartialChecker = sourceIdHelper(_.supportsPutPartial) _
+  val supportsHintChecker       = sourceIdHelper(_.supportsHint)       _
+
+  // TODO: Merge sourceIdHelper2 with sourceIdHelper


Please resolve this

src/main/scala/tilelink/Parameters.scala

…ket-chip into supporthelperchange

hcook · 2020-07-16T19:25:36Z

@jchang0 @brrmorre Because the diplomacy information for the edge contains the static tile id, this means that the monitors on the tile master crossbar and master ports breaks deduplication of multicore tiles...

jackkoenig · 2020-07-16T20:51:58Z

Chisel 3.4 should support instance-specific annotations no longer breaking deduplication. TBD how to handle annotated signals that get constant propagated, but this should make it possible make things like monitors link against the DUT via annotations and thus not break dedup when they have custom code per instance.

Just mentioning some nearish possibilities, but for now I think we need to remove that part of the info String.

* un-deprecate TLMasterSlavePortParameters.supports* * move expectsVipCheckerMaster/Slave into Monitor.scala from Parameters.scala * remove TLMasterPortParameters.expectsVipChecker * remove TLSlavePortParameters.expectsVipChecker * TileLink Monitor.scala: addressHelper supports/emit Safe, not Fast (assumed address exists) * TileLink Parameters.scala: unresolved feedback from PR (#2505) * remove deprecated TLMasterParameters.supports*

brrmorre and others added 4 commits June 1, 2020 14:39

started support helper fix

c873182

saving my progress

f2137e0

fixed calls

7e50597

Comments

ddcf4ef

brrmorre self-assigned this Jun 5, 2020

brrmorre added the draft Used by mergify.io automated merging rules label Jun 5, 2020

brrmorre added 4 commits June 8, 2020 13:28

update monitor to use my new functions

af5c09d

changed manager to slave and client to master in parameters and chang…

29d5b71

…ed function calls in monitors

fixed bugs

65cf93a

changed sourceID to sourceId and address to sourceId

2868566

jchang0 reviewed Jun 9, 2020

View reviewed changes

hcook reviewed Jun 9, 2020

View reviewed changes

src/main/scala/tilelink/Parameters.scala Show resolved Hide resolved

hcook reviewed Jun 9, 2020

View reviewed changes

brrmorre added 5 commits June 11, 2020 14:24

update

a5cd26c

clean up a bit

201a8c2

cleaned up comments

32796bf

resolving bugs with changing the names

1189a23

update

ea1b6fb

jchang0 reviewed Jun 12, 2020

View reviewed changes

brrmorre commented Jun 15, 2020

View reviewed changes