-
Notifications
You must be signed in to change notification settings - Fork 602
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
Define Data .toString #985
Changes from 10 commits
a872cdb
5f0fdae
f474468
c65fbfc
e1efde0
5ad66ec
b0eb007
fc6a23e
c6a9519
40b5eef
38b1542
0469462
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
// See LICENSE for license details. | ||
|
||
package chiselTests | ||
|
||
import org.scalatest._ | ||
|
||
import chisel3._ | ||
import chisel3.experimental.{ChiselEnum, FixedPoint, RawModule, MultiIOModule} | ||
|
||
class DataPrintSpec extends ChiselFlatSpec with Matchers { | ||
object EnumTest extends ChiselEnum { | ||
val sNone, sOne, sTwo = Value | ||
} | ||
|
||
// TODO: dedup w/ BundleLiteralSpec | ||
class BundleTest extends Bundle { | ||
val a = UInt(8.W) | ||
val b = Bool() | ||
|
||
// Bundle literal constructor code, which will be auto-generated using macro annotations in | ||
// the future. | ||
import chisel3.core.BundleLitBinding | ||
import chisel3.internal.firrtl.{ULit, Width} | ||
// Full bundle literal constructor | ||
def Lit(aVal: UInt, bVal: Bool): BundleTest = { | ||
val clone = cloneType | ||
clone.selfBind(BundleLitBinding(Map( | ||
clone.a -> litArgOfBits(aVal), | ||
clone.b -> litArgOfBits(bVal) | ||
))) | ||
clone | ||
} | ||
} | ||
|
||
"Data types" should "have a meaningful string representation" in { | ||
elaborate { new RawModule { | ||
UInt().toString should be ("UInt") | ||
UInt(8.W).toString should be ("UInt<8>") | ||
SInt(15.W).toString should be ("SInt<15>") | ||
Bool().toString should be ("Bool") | ||
Clock().toString should be ("Clock") | ||
FixedPoint(5.W, 3.BP).toString should be ("FixedPoint<5><<3>>") | ||
Vec(3, UInt(2.W)).toString should be ("UInt<2>[3]") | ||
EnumTest.Type().toString should be ("EnumTest") | ||
(new BundleTest).toString should be ("BundleTest") | ||
} } | ||
} | ||
|
||
class BoundDataModule extends MultiIOModule { // not in the test to avoid anon naming suffixes | ||
Wire(UInt()).toString should be("UInt(Wire in DataPrintSpec$BoundDataModule)") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think it might be worth having a source locator for Data? Let people know where the Wire was constructed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly. This is new enough that I don't know what the use case for this will be (other than printing literals), so I'd like to wait and see first. There would also need to be infrastructure built for it, mainly IO needs to have a source locator attached to it, and all Bindings needs a copy of the source locator (currently is only part of the FIRRTL command). Adding source locators for literals would be more difficult because of the possibility for a chained apply, so we might have to build more source info transform macros. |
||
Reg(SInt()).toString should be("SInt(Reg in DataPrintSpec$BoundDataModule)") | ||
val io = IO(Output(Bool())) // needs a name so elaboration doesn't fail | ||
io.toString should be("Bool(IO in unelaborated DataPrintSpec$BoundDataModule)") | ||
val m = Mem(4, UInt(2.W)) | ||
m(2).toString should be("UInt<2>(MemPort in DataPrintSpec$BoundDataModule)") | ||
(2.U + 2.U).toString should be("UInt<2>(OpResult in DataPrintSpec$BoundDataModule)") | ||
Wire(Vec(3, UInt(2.W))).toString should be ("UInt<2>[3](Wire in DataPrintSpec$BoundDataModule)") | ||
|
||
class InnerModule extends MultiIOModule { | ||
val io = IO(Output(new Bundle { | ||
val a = UInt(4.W) | ||
})) | ||
} | ||
val inner = Module(new InnerModule) | ||
inner.clock.toString should be ("Clock(IO clock in DataPrintSpec$BoundDataModule$InnerModule)") | ||
inner.io.a.toString should be ("UInt<4>(IO io_a in DataPrintSpec$BoundDataModule$InnerModule)") | ||
} | ||
|
||
"Bound data types" should "have a meaningful string representation" in { | ||
elaborate { new BoundDataModule } | ||
} | ||
|
||
"Literals" should "have a meaningful string representation" in { | ||
elaborate { new RawModule { | ||
3.U.toString should be ("UInt<2>(3)") | ||
3.U(5.W).toString should be ("UInt<5>(3)") | ||
-1.S.toString should be ("SInt<1>(-1)") | ||
false.B.toString should be ("Bool(false)") | ||
true.B.toString should be ("Bool(true)") | ||
2.25.F(6.W, 2.BP).toString should be ("FixedPoint<6><<2>>(2.25)") | ||
-2.25.F(6.W, 2.BP).toString should be ("FixedPoint<6><<2>>(-2.25)") | ||
EnumTest.sNone.toString should be ("EnumTest(0=sNone)") | ||
EnumTest.sTwo.toString should be ("EnumTest(2=sTwo)") | ||
EnumTest(1.U).toString should be ("EnumTest(1=sOne)") | ||
(new BundleTest).Lit(2.U, false.B).toString should be ("BundleTest(a=UInt<8>(2), b=Bool(false))") | ||
new Bundle { | ||
val a = UInt(8.W) | ||
}.toString should be ("Bundle") | ||
DontCare.toString should be ("DontCare()") | ||
} } | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
className
is used byPrintable
for hardware pretty printing. I think just using the default Aggregate.getClass.getSimpleName
is better for obvious reasons. I can't recall my original thinking, but I think it had to do with making this pretty printing look like printing a Map in Scala (since that's sort of what Records/Bundles are), but I think giving the name of the Bundle class is generally better.My only question is what about anonymous or inner bundles? You'll get some pretty weird names but perhaps that's still desirable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the intention of this is for human debugging, I think providing better naming on a best-effort basis makes sense. So anonymous Bundles will probably give some nasty name. Inner bundles should work fine, it's one of the test cases (though not intentionally).
Random thought: maybe we could regex match anonymous bundle names and replace it with a more friendly AnonymousBundle? Might be more trouble than it's worth though, so I'd prefer to wait for user feedback first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that's sensible, at least
MyModule$anon$1
tells you more than justBundle
!