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.

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

Bundle Literals Framework #820

Merged
merged 25 commits into from
Jul 4, 2018
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
fbad34b
Infrastructure for bundle literals
ducky64 May 2, 2018
c23faf0
work on new style literal accessors
ducky64 May 10, 2018
c2a51df
refactoring of lit and ref implementations
ducky64 May 10, 2018
ab080ea
bundle literal mockup, but broken =(
ducky64 May 10, 2018
20634c9
lol=(
ducky64 May 10, 2018
75d0ccf
Run-unique ids
ducky64 May 11, 2018
927e604
style
ducky64 May 11, 2018
4948abe
delete debugging stuff
ducky64 May 11, 2018
fb53b1e
broken
ducky64 May 11, 2018
aa927ba
still broken
ducky64 May 11, 2018
761fad6
unbroken
ducky64 May 11, 2018
f109803
Add new test LitInsideOutsideTester
chick May 23, 2018
d655175
Comment out assertion test, fix ref generation
ducky64 May 24, 2018
f888adf
Add BundleLiteralSpec
ducky64 May 24, 2018
7fa963d
properly fix undefined clock/reset issues
ducky64 Jun 28, 2018
907c27a
binding => topBinding so that partial Bundles work and undefined Bund…
ducky64 Jun 28, 2018
2a82c99
Style fixes
ducky64 Jun 29, 2018
418d7cc
Change [public] Data.elementLitArg => [protected] Aggregate.litArgOfBits
ducky64 Jun 29, 2018
9a3cd9f
Prefer litValue, eliminate litToBigInt
ducky64 Jul 1, 2018
0292722
Add test that UInt, SInt, and FP literals do not impact naming
jackkoenig Jul 3, 2018
93eedef
Remove forceName rom BlackBox/ExtModule, filter out forceName in User…
ducky64 Jul 3, 2018
ac34d81
Merge branch 'bundlelits' of https://github.com/freechipsproject/chis…
ducky64 Jul 3, 2018
62603b0
Fix strict namer
ducky64 Jul 4, 2018
97907ca
Change wording of internal failure
ducky64 Jul 4, 2018
4c652c7
Merge branch 'master' into bundlelits
ducky64 Jul 4, 2018
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
8 changes: 7 additions & 1 deletion chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ sealed abstract class Aggregate extends Data {
}
}

override def litOption = ??? // TODO implement me
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this TODO intended to be resolved in this PR or later?

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 is meant to be implemented later. Literal extractors on Bundles would be a new feature.


// Returns the LitArg of a Bits object.
// Internal API for Bundle literals, to copy the LitArg of argument literals into the top map.
protected def litArgOfBits(elt: Bits): LitArg = elt.litArgOption.get

/** Returns a Seq of the immediate contents of this Aggregate, in order.
*/
def getElements: Seq[Data]
Expand Down Expand Up @@ -736,7 +742,7 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record {
// (which could lead to data conflicts, since it's likely the user didn't know to re-bind them).
// This is not guaranteed to catch all cases (for example, Data in Tuples or Iterables).
val boundDataParamNames = ctorParamsNameVals.collect {
case (paramName, paramVal: Data) if paramVal.hasBinding => paramName
case (paramName, paramVal: Data) if paramVal.topBindingOpt.isDefined => paramName
}
if (boundDataParamNames.nonEmpty) {
autoClonetypeError(s"constructor parameters (${boundDataParamNames.sorted.mkString(", ")}) have values that are hardware types, which is likely to cause subtle errors." +
Expand Down
4 changes: 2 additions & 2 deletions chiselFrontend/src/main/scala/chisel3/core/Assert.scala
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ object assert { // scalastyle:ignore object.name
case None => s"Assertion failed\n at $escLine\n"
}
printf.printfWithoutReset(fmt, data:_*)
pushCommand(Stop(sourceInfo, Node(Builder.forcedClock), 1))
pushCommand(Stop(sourceInfo, Builder.forcedClock.ref, 1))
}
}

Expand All @@ -81,7 +81,7 @@ object stop { // scalastyle:ignore object.name
/** Terminate execution with a failure code. */
def apply(code: Int)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Unit = {
when (!Module.reset.toBool) {
pushCommand(Stop(sourceInfo, Node(Builder.forcedClock), code))
pushCommand(Stop(sourceInfo, Builder.forcedClock.ref, code))
}
}

Expand Down
4 changes: 2 additions & 2 deletions chiselFrontend/src/main/scala/chisel3/core/BiConnect.scala
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ object BiConnect {
// Source and sink are ambiguous in the case of a Bi/Bulk Connect (<>).
// If either is a DontCareBinding, just issue a DefInvalid for the other,
// otherwise, issue a Connect.
(left.binding, right.binding) match {
(left.topBinding, right.topBinding) match {
case (lb: DontCareBinding, _) => pushCommand(DefInvalid(sourceInfo, right.lref))
case (_, rb: DontCareBinding) => pushCommand(DefInvalid(sourceInfo, left.lref))
case (_, _) => pushCommand(Connect(sourceInfo, right.lref, left.ref))
Expand All @@ -197,7 +197,7 @@ object BiConnect {
// Source and sink are ambiguous in the case of a Bi/Bulk Connect (<>).
// If either is a DontCareBinding, just issue a DefInvalid for the other,
// otherwise, issue a Connect.
(left.binding, right.binding) match {
(left.topBinding, right.topBinding) match {
case (lb: DontCareBinding, _) => pushCommand(DefInvalid(sourceInfo, right.lref))
case (_, rb: DontCareBinding) => pushCommand(DefInvalid(sourceInfo, left.lref))
case (_, _) => pushCommand(Connect(sourceInfo, left.lref, right.ref))
Expand Down
15 changes: 10 additions & 5 deletions chiselFrontend/src/main/scala/chisel3/core/Binding.scala
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package chisel3.core

import chisel3.internal.Builder.{forcedModule}
import chisel3.internal.firrtl.LitArg

object Binding {
class BindingException(message: String) extends Exception(message)
Expand All @@ -26,7 +27,7 @@ object requireIsHardware {
case Some(x: BaseModule) => x._compatAutoWrapPorts
case _ =>
}
if (!node.hasBinding) {
if (!node.topBindingOpt.isDefined) {
val prefix = if (msg.nonEmpty) s"$msg " else ""
throw Binding.ExpectedHardwareException(s"$prefix'$node' must be hardware, " +
"not a bare Chisel type. Perhaps you forgot to wrap it in Wire(_) or IO(_)?")
Expand All @@ -37,7 +38,7 @@ object requireIsHardware {
/** Requires that a node is a chisel type (not hardware, "unbound")
*/
object requireIsChiselType {
def apply(node: Data, msg: String = "") = if (node.hasBinding) {
def apply(node: Data, msg: String = "") = if (node.topBindingOpt.isDefined) {
val prefix = if (msg.nonEmpty) s"$msg " else ""
throw Binding.ExpectedChiselTypeException(s"$prefix'$node' must be a Chisel type, not hardware")
}
Expand Down Expand Up @@ -92,8 +93,6 @@ sealed trait ConstrainedBinding extends TopBinding {
// A binding representing a data that cannot be (re)assigned to.
sealed trait ReadOnlyBinding extends TopBinding

// TODO literal info here
case class LitBinding() extends UnconstrainedBinding with ReadOnlyBinding
// TODO(twigg): Ops between unenclosed nodes can also be unenclosed
// However, Chisel currently binds all op results to a module
case class OpBinding(enclosure: UserModule) extends ConstrainedBinding with ReadOnlyBinding
Expand All @@ -103,8 +102,14 @@ case class RegBinding(enclosure: UserModule) extends ConstrainedBinding
case class WireBinding(enclosure: UserModule) extends ConstrainedBinding

case class ChildBinding(parent: Data) extends Binding {
def location = parent.binding.location
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

sealed trait LitBinding extends UnconstrainedBinding with ReadOnlyBinding
// Literal binding attached to a element that is not part of a Bundle.
case class ElementLitBinding(litArg: LitArg) extends LitBinding
// Literal binding attached to the root of a Bundle, containing literal values of its children.
case class BundleLitBinding(litMap: Map[Data, LitArg]) extends LitBinding
98 changes: 63 additions & 35 deletions chiselFrontend/src/main/scala/chisel3/core/Bits.scala
Original file line number Diff line number Diff line change
Expand Up @@ -61,20 +61,44 @@ private[chisel3] sealed trait ToBoolable extends Element {
* bitwise operations.
*/
//scalastyle:off number.of.methods
sealed abstract class Bits(width: Width, override val litArg: Option[LitArg])
sealed abstract class Bits(width: Width)
extends Element(width) with ToBoolable {
// TODO: perhaps make this concrete?
// Arguments for: self-checking code (can't do arithmetic on bits)
// Arguments against: generates down to a FIRRTL UInt anyways

// If this is a literal, setRef so that we don't allocate a name
litArg.foreach(setRef)

// Only used for in a few cases, hopefully to be removed
private[core] def cloneTypeWidth(width: Width): this.type

def cloneType: this.type = cloneTypeWidth(width)

private[core] override def topBindingOpt: Option[TopBinding] = super.topBindingOpt match {
// Translate Bundle lit bindings to Element lit bindings
case Some(BundleLitBinding(litMap)) => litMap.get(this) match {
case Some(litArg) => Some(ElementLitBinding(litArg))
case _ => Some(DontCareBinding())
}
case topBindingOpt => topBindingOpt
}

private[core] def litArgOption: Option[LitArg] = topBindingOpt match {
case Some(ElementLitBinding(litArg)) => Some(litArg)
case _ => None
}

override def litOption: Option[BigInt] = litArgOption.map(_.num)
private[core] def litIsForcedWidth: Option[Boolean] = litArgOption.map(_.forcedWidth)

// provide bits-specific literal handling functionality here
override private[chisel3] def ref: Arg = topBindingOpt match {
case Some(ElementLitBinding(litArg)) => litArg
case Some(BundleLitBinding(litMap)) => litMap.get(this) match {
case Some(litArg) => litArg
case _ => throwException(s"internal error: DontCare should be caught before getting ref")
}
case _ => super.ref
}

final def tail(n: Int): UInt = macro SourceInfoTransform.nArg
final def head(n: Int): UInt = macro SourceInfoTransform.nArg

Expand Down Expand Up @@ -106,13 +130,8 @@ sealed abstract class Bits(width: Width, override val litArg: Option[LitArg])
if (x < 0) {
Builder.error(s"Negative bit indices are illegal (got $x)")
}
if (isLit()) {
(((litValue() >> x.toInt) & 1) == 1).asBool
} else {

requireIsHardware(this, "bits to be indexed")
pushOp(DefPrim(sourceInfo, Bool(), BitsExtractOp, this.ref, ILit(x), ILit(x)))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for the historical record:

These are minor constant propagations done on literals. I did some archaeology to understand why they were there in the first place. They date to very early chisel3 7ada20e and appear to be largely to work around bugs in order to get riscv-mini compiling. Most of them have already been removed but a couple seem to have stuck around. I agree they should be removed.

requireIsHardware(this, "bits to be indexed")
pushOp(DefPrim(sourceInfo, Bool(), BitsExtractOp, this.ref, ILit(x), ILit(x)))
}

/** Returns the specified bit on this wire as a [[Bool]], statically
Expand Down Expand Up @@ -150,12 +169,8 @@ sealed abstract class Bits(width: Width, override val litArg: Option[LitArg])
Builder.error(s"Invalid bit range ($x,$y)")
}
val w = x - y + 1
if (isLit()) {
((litValue >> y) & ((BigInt(1) << w) - 1)).asUInt(w.W)
} else {
requireIsHardware(this, "bits to be sliced")
pushOp(DefPrim(sourceInfo, UInt(Width(w)), BitsExtractOp, this.ref, ILit(x), ILit(y)))
}
requireIsHardware(this, "bits to be sliced")
pushOp(DefPrim(sourceInfo, UInt(Width(w)), BitsExtractOp, this.ref, ILit(x), ILit(y)))
}

// REVIEW TODO: again, is this necessary? Or just have this and use implicits?
Expand Down Expand Up @@ -295,7 +310,7 @@ sealed abstract class Bits(width: Width, override val litArg: Option[LitArg])
implicit val sourceInfo = DeprecatedSourceInfo
do_asSInt
}

@chiselRuntimeDeprecated
@deprecated("Use asUInt, which makes the reinterpret cast more explicit", "chisel3")
final def toUInt(implicit compileOptions: CompileOptions): UInt = {
Expand Down Expand Up @@ -428,8 +443,7 @@ abstract trait Num[T <: Data] {
/** A data type for unsigned integers, represented as a binary bitvector.
* Defines arithmetic operations between other integer types.
*/
sealed class UInt private[core] (width: Width, lit: Option[ULit] = None)
extends Bits(width, lit) with Num[UInt] {
sealed class UInt private[core] (width: Width) extends Bits(width) with Num[UInt] {

private[core] override def typeEquivalent(that: Data): Boolean =
that.isInstanceOf[UInt] && this.width == that.width
Expand Down Expand Up @@ -576,9 +590,9 @@ trait UIntFactory {
/** Create a UInt literal with specified width. */
protected[chisel3] def Lit(value: BigInt, width: Width): UInt = {
val lit = ULit(value, width)
val result = new UInt(lit.width, Some(lit))
val result = new UInt(lit.width)
// Bind result to being an Literal
result.bind(LitBinding())
result.bind(ElementLitBinding(lit))
result
}

Expand All @@ -595,8 +609,7 @@ trait UIntFactory {
object UInt extends UIntFactory
object Bits extends UIntFactory

sealed class SInt private[core] (width: Width, lit: Option[SLit] = None)
extends Bits(width, lit) with Num[SInt] {
sealed class SInt private[core] (width: Width) extends Bits(width) with Num[SInt] {

private[core] override def typeEquivalent(that: Data): Boolean =
this.getClass == that.getClass && this.width == that.width // TODO: should this be true for unspecified widths?
Expand Down Expand Up @@ -731,9 +744,8 @@ trait SIntFactory {
/** Create an SInt literal with specified width. */
protected[chisel3] def Lit(value: BigInt, width: Width): SInt = {
val lit = SLit(value, width)
val result = new SInt(lit.width, Some(lit))
// Bind result to being an Literal
result.bind(LitBinding())
val result = new SInt(lit.width)
result.bind(ElementLitBinding(lit))
result
}
}
Expand All @@ -746,12 +758,20 @@ sealed trait Reset extends Element with ToBoolable
// operations on a Bool make sense?
/** A data type for booleans, defined as a single bit indicating true or false.
*/
sealed class Bool(lit: Option[ULit] = None) extends UInt(1.W, lit) with Reset {
sealed class Bool() extends UInt(1.W) with Reset {
private[core] override def cloneTypeWidth(w: Width): this.type = {
require(!w.known || w.get == 1)
new Bool().asInstanceOf[this.type]
}

def litToBooleanOption: Option[Boolean] = litOption.map {
case intVal if intVal == 1 => true
case intVal if intVal == 0 => false
case intVal => throwException(s"Boolean with unexpected literal value $intVal")
}
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 better as something like:

def litToBooleanOption: Option[Boolean] = litToBigIntOption.map { intVal =>
  if (intVal == 1) true
  else if (intVal == 0) false
  else throwException(s"Boolean with unexpected literal value $intVal")
}

or

def litToBooleanOption: Option[Boolean] = litToBigIntOption.map {
  case intVal if intVal == 1 => true
  case intVal if intVal == 0 => false
  case intVal => throwException(s"Boolean with unexpected literal value $intVal")
}

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 like option 2 for the (grammatical) parallelism; fixed.


def litToBoolean: Boolean = litToBooleanOption.get

// REVIEW TODO: Why does this need to exist and have different conventions
// than Bits?
final def & (that: Bool): Bool = macro SourceInfoTransform.thatArg
Expand Down Expand Up @@ -795,9 +815,8 @@ trait BoolFactory {
/** Creates Bool literal.
*/
protected[chisel3] def Lit(x: Boolean): Bool = {
val result = new Bool(Some(ULit(if (x) 1 else 0, Width(1))))
// Bind result to being an Literal
result.bind(LitBinding())
val result = new Bool()
result.bind(ElementLitBinding(ULit(if (x) 1 else 0, Width(1))))
result
}
}
Expand All @@ -817,8 +836,8 @@ object Bool extends BoolFactory
* and thus use this field as a simple exponent
* @param lit
*/
sealed class FixedPoint private (width: Width, val binaryPoint: BinaryPoint, lit: Option[FPLit] = None)
extends Bits(width, lit) with Num[FixedPoint] {
sealed class FixedPoint private (width: Width, val binaryPoint: BinaryPoint)
extends Bits(width) with Num[FixedPoint] {
private[core] override def typeEquivalent(that: Data): Boolean = that match {
case that: FixedPoint => this.width == that.width && this.binaryPoint == that.binaryPoint // TODO: should this be true for unspecified widths?
case _ => false
Expand All @@ -832,6 +851,13 @@ sealed class FixedPoint private (width: Width, val binaryPoint: BinaryPoint, lit
case _ => this badConnect that
}

def litToDoubleOption: Option[Double] = litOption.map { intVal =>
val multiplier = math.pow(2, binaryPoint.get)
intVal.toDouble / multiplier
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess across the board I think it's better to leave off the case _ => None by just using .map on Options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


def litToDouble: Double = litToDoubleOption.get

final def unary_- (): FixedPoint = macro SourceInfoTransform.noArg
final def unary_-% (): FixedPoint = macro SourceInfoTransform.noArg

Expand Down Expand Up @@ -1047,8 +1073,8 @@ object FixedPoint {
/** Create an FixedPoint port with specified width and binary position. */
def apply(value: BigInt, width: Width, binaryPoint: BinaryPoint): FixedPoint = {
val lit = FPLit(value, width, binaryPoint)
val newLiteral = new FixedPoint(lit.width, lit.binaryPoint, Some(lit))
newLiteral.bind(LitBinding())
val newLiteral = new FixedPoint(lit.width, lit.binaryPoint)
newLiteral.bind(ElementLitBinding(lit))
newLiteral
}

Expand Down Expand Up @@ -1099,6 +1125,8 @@ final class Analog private (width: Width) extends Element(width) {
private[core] override def typeEquivalent(that: Data): Boolean =
that.isInstanceOf[Analog] && this.width == that.width

override def litOption = None

def cloneType: this.type = new Analog(width).asInstanceOf[this.type]

// Used to enforce single bulk connect of Analog types, multi-attach is still okay
Expand Down
2 changes: 0 additions & 2 deletions chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ abstract class ExtModule(val params: Map[String, Param] = Map.empty[String, Para
// While BlackBoxes are not supposed to have an implementation, we still need to call
// _onModuleClose on all nodes (for example, Aggregates use it for recursive naming).
for (id <- getIds) {
id.forceName(default="_T", _namespace)
id._onModuleClose
}

Expand Down Expand Up @@ -155,7 +154,6 @@ abstract class BlackBox(val params: Map[String, Param] = Map.empty[String, Param
// Doing so would cause the wrong names to be assigned, since their parent
// is now the module itself instead of the io bundle.
for (id <- getIds; if id ne io) {
id.forceName(default="_T", _namespace)
id._onModuleClose
}

Expand Down
2 changes: 2 additions & 0 deletions chiselFrontend/src/main/scala/chisel3/core/Clock.scala
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ sealed class Clock extends Element(Width(1)) {
case _ => super.badConnect(that)(sourceInfo)
}

override def litOption = None

/** Not really supported */
def toPrintable: Printable = PString("CLOCK")

Expand Down
Loading