From 69e8250dd47210cee809c9ae231c1e320d76c084 Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Fri, 29 Mar 2019 15:10:13 -0700 Subject: Ignore empty aggregates elements when binding aggregate direction (#946) Previously, including an empty aggregate in a Bundle would cause a MixedDirectionAggregateException because it has no elements and thus doesn't have a direction * Add SampleElementBinding for Vec sample elements * Add ActualDirection.Empty for bound empty aggregates--- .../src/main/scala/chisel3/core/Aggregate.scala | 76 +++++++++++----------- .../src/main/scala/chisel3/core/Binding.scala | 6 +- .../src/main/scala/chisel3/core/Bits.scala | 6 +- .../src/main/scala/chisel3/core/Data.scala | 51 ++++++++++++++- .../src/main/scala/chisel3/core/StrongEnum.scala | 2 +- 5 files changed, 92 insertions(+), 49 deletions(-) (limited to 'chiselFrontend/src/main') diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala index b2f01c2a..7a6b9b56 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala @@ -16,7 +16,7 @@ import chisel3.SourceInfoDoc * of) other Data objects. */ sealed abstract class Aggregate extends Data { - private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection) { // scalastyle:ignore cyclomatic.complexity line.size.limit + private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection) { binding = target val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection) @@ -25,41 +25,13 @@ sealed abstract class Aggregate extends Data { } // Check that children obey the directionality rules. - val childDirections = getElements.map(_.direction).toSet - direction = if (childDirections == Set()) { // Sadly, Scala can't do set matching - // If empty, use my assigned direction - resolvedDirection match { - case SpecifiedDirection.Unspecified | SpecifiedDirection.Flip => ActualDirection.Unspecified - case SpecifiedDirection.Output => ActualDirection.Output - case SpecifiedDirection.Input => ActualDirection.Input - } - } else if (childDirections == Set(ActualDirection.Unspecified)) { - ActualDirection.Unspecified - } else if (childDirections == Set(ActualDirection.Input)) { - ActualDirection.Input - } else if (childDirections == Set(ActualDirection.Output)) { - ActualDirection.Output - } else if (childDirections subsetOf - Set(ActualDirection.Output, ActualDirection.Input, - ActualDirection.Bidirectional(ActualDirection.Default), - ActualDirection.Bidirectional(ActualDirection.Flipped))) { - resolvedDirection match { - case SpecifiedDirection.Unspecified => ActualDirection.Bidirectional(ActualDirection.Default) - case SpecifiedDirection.Flip => ActualDirection.Bidirectional(ActualDirection.Flipped) - case _ => throw new RuntimeException("Unexpected forced Input / Output") - } - } else { - this match { - // Anything flies in compatibility mode - case t: Record if !t.compileOptions.dontAssumeDirectionality => resolvedDirection match { - case SpecifiedDirection.Unspecified => ActualDirection.Bidirectional(ActualDirection.Default) - case SpecifiedDirection.Flip => ActualDirection.Bidirectional(ActualDirection.Flipped) - case _ => ActualDirection.Bidirectional(ActualDirection.Default) - } - case _ => - val childWithDirections = getElements zip getElements.map(_.direction) - throw Binding.MixedDirectionAggregateException(s"Aggregate '$this' can't have elements that are both directioned and undirectioned: $childWithDirections") // scalastyle:ignore line.size.limit - } + val childDirections = getElements.map(_.direction).toSet - ActualDirection.Empty + direction = ActualDirection.fromChildren(childDirections, resolvedDirection) match { + case Some(dir) => dir + case None => + val childWithDirections = getElements zip getElements.map(_.direction) + throw Binding.MixedDirectionAggregateException( + s"Aggregate '$this' can't have elements that are both directioned and undirectioned: $childWithDirections") } } @@ -164,10 +136,26 @@ sealed class Vec[T <: Data] private[core] (gen: => T, val length: Int) case _ => false } + private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection) { + binding = target + + val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection) + sample_element.bind(SampleElementBinding(this), resolvedDirection) + for (child <- getElements) { // assume that all children are the same + child.bind(ChildBinding(this), resolvedDirection) + } + + // Note: this differs from the Aggregate.bind behavior on an empty Vec, since this looks at the + // sample element instead of actual elements. + direction = sample_element.direction + } + // Note: the constructor takes a gen() function instead of a Seq to enforce // that all elements must be the same and because it makes FIRRTL generation // simpler. private val self: Seq[T] = Vector.fill(length)(gen) + for ((elt, i) <- self.zipWithIndex) + elt.setRef(this, i) /** * sample_element 'tracks' all changes to the elements. @@ -253,9 +241,6 @@ sealed class Vec[T <: Data] private[core] (gen: => T, val length: Int) override def getElements: Seq[Data] = (0 until length).map(apply(_)) - for ((elt, i) <- self.zipWithIndex) - elt.setRef(this, i) - /** Default "pretty-print" implementation * Analogous to printing a Seq * Results in "Vec(elt0, elt1, ...)" @@ -441,6 +426,19 @@ trait VecLike[T <: Data] extends collection.IndexedSeq[T] with HasId with Source * RTL writers should use [[Bundle]]. See [[Record#elements]] for an example. */ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptions) extends Aggregate { + private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection): Unit = { + try { + super.bind(target, parentDirection) + } catch { // nasty compatibility mode shim, where anything flies + case e: Binding.MixedDirectionAggregateException if !compileOptions.dontAssumeDirectionality => + val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection) + direction = resolvedDirection match { + case SpecifiedDirection.Unspecified => ActualDirection.Bidirectional(ActualDirection.Default) + case SpecifiedDirection.Flip => ActualDirection.Bidirectional(ActualDirection.Flipped) + case _ => ActualDirection.Bidirectional(ActualDirection.Default) + } + } + } /** The collection of [[Data]] * diff --git a/chiselFrontend/src/main/scala/chisel3/core/Binding.scala b/chiselFrontend/src/main/scala/chisel3/core/Binding.scala index 55e6bcf9..37ce3f66 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Binding.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Binding.scala @@ -87,7 +87,7 @@ sealed trait UnconstrainedBinding extends TopBinding { def location: Option[BaseModule] = None } // A constrained binding can only be read/written by specific modules -// Location will track where this Module is +// Location will track where this Module is, and the bound object can be referenced in FIRRTL sealed trait ConstrainedBinding extends TopBinding { def enclosure: BaseModule def location: Option[BaseModule] = Some(enclosure) @@ -107,6 +107,10 @@ case class WireBinding(enclosure: RawModule) extends ConstrainedBinding case class ChildBinding(parent: Data) extends Binding { def location: Option[BaseModule] = parent.topBinding.location } +/** Special binding for Vec.sample_element */ +case class SampleElementBinding[T <: Data](parent: Vec[T]) extends Binding { + def location = parent.topBinding.location +} // A DontCare element has a specific Binding, somewhat like a literal. // It is a source (RHS). It may only be connected/applied to sinks. case class DontCareBinding() extends UnconstrainedBinding diff --git a/chiselFrontend/src/main/scala/chisel3/core/Bits.scala b/chiselFrontend/src/main/scala/chisel3/core/Bits.scala index 7270d92b..43a851ed 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Bits.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Bits.scala @@ -27,11 +27,7 @@ abstract class Element extends Data { private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection) { binding = target val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection) - direction = resolvedDirection match { - case SpecifiedDirection.Unspecified | SpecifiedDirection.Flip => ActualDirection.Unspecified - case SpecifiedDirection.Output => ActualDirection.Output - case SpecifiedDirection.Input => ActualDirection.Input - } + direction = ActualDirection.fromSpecified(resolvedDirection) } private[core] override def topBindingOpt: Option[TopBinding] = super.topBindingOpt match { diff --git a/chiselFrontend/src/main/scala/chisel3/core/Data.scala b/chiselFrontend/src/main/scala/chisel3/core/Data.scala index daa89045..03c71af5 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Data.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Data.scala @@ -54,6 +54,10 @@ object SpecifiedDirection { sealed abstract class ActualDirection object ActualDirection { + /** The object does not exist / is empty and hence has no direction + */ + case object Empty extends ActualDirection + /** Undirectioned, struct-like */ case object Unspecified extends ActualDirection @@ -69,6 +73,42 @@ object ActualDirection { case object Flipped extends BidirectionalDirection case class Bidirectional(dir: BidirectionalDirection) extends ActualDirection + + def fromSpecified(direction: SpecifiedDirection): ActualDirection = direction match { + case SpecifiedDirection.Unspecified | SpecifiedDirection.Flip => ActualDirection.Unspecified + case SpecifiedDirection.Output => ActualDirection.Output + case SpecifiedDirection.Input => ActualDirection.Input + } + + /** Determine the actual binding of a container given directions of its children. + * Returns None in the case of mixed specified / unspecified directionality. + */ + def fromChildren(childDirections: Set[ActualDirection], containerDirection: SpecifiedDirection): + Option[ActualDirection] = { + if (childDirections == Set()) { // Sadly, Scala can't do set matching + ActualDirection.fromSpecified(containerDirection) match { + case ActualDirection.Unspecified => Some(ActualDirection.Empty) // empty direction if relative / no direction + case dir => Some(dir) // use assigned direction if specified + } + } else if (childDirections == Set(ActualDirection.Unspecified)) { + Some(ActualDirection.Unspecified) + } else if (childDirections == Set(ActualDirection.Input)) { + Some(ActualDirection.Input) + } else if (childDirections == Set(ActualDirection.Output)) { + Some(ActualDirection.Output) + } else if (childDirections subsetOf + Set(ActualDirection.Output, ActualDirection.Input, + ActualDirection.Bidirectional(ActualDirection.Default), + ActualDirection.Bidirectional(ActualDirection.Flipped))) { + containerDirection match { + case SpecifiedDirection.Unspecified => Some(ActualDirection.Bidirectional(ActualDirection.Default)) + case SpecifiedDirection.Flip => Some(ActualDirection.Bidirectional(ActualDirection.Flipped)) + case _ => throw new RuntimeException("Unexpected forced Input / Output") + } + } else { + None + } + } } object debug { // scalastyle:ignore object.name @@ -274,9 +314,14 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc { // sc _binding = Some(target) } - private[core] def topBindingOpt: Option[TopBinding] = _binding.map { - case ChildBinding(parent) => parent.topBinding - case bindingVal: TopBinding => bindingVal + private[core] def topBindingOpt: Option[TopBinding] = _binding.flatMap { + case ChildBinding(parent) => parent.topBindingOpt + case bindingVal: TopBinding => Some(bindingVal) + case _: SampleElementBinding[_] => None + // TODO: technically, it's bound, but it's more of a ghost binding and None is probably the most appropriate + // Note: sample elements should not be user-accessible, so there's not really a good reason to access its + // top binding. However, we can't make this assert out right not because a larger refactoring is needed. + // See https://github.com/freechipsproject/chisel3/pull/946 } private[core] def topBinding: TopBinding = topBindingOpt.get diff --git a/chiselFrontend/src/main/scala/chisel3/core/StrongEnum.scala b/chiselFrontend/src/main/scala/chisel3/core/StrongEnum.scala index 3439cf16..c359a119 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/StrongEnum.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/StrongEnum.scala @@ -161,7 +161,7 @@ abstract class EnumType(private val factory: EnumFactory, selfAnnotating: Boolea super.bind(target, parentDirection) // Make sure we only annotate hardware and not literals - if (selfAnnotating && litOption.isEmpty) { + if (selfAnnotating && topBindingOpt.isDefined && topBindingOpt.get.isInstanceOf[ConstrainedBinding]) { annotateEnum() } } -- cgit v1.2.3