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

Remove deprecations #309

Merged
merged 3 commits into from
Feb 4, 2021

Conversation

sequencer
Copy link
Member

@sequencer sequencer commented Jan 15, 2021

This PR depends on #308
chisel-testers won't be a blocker on chipsalliance/chisel#1730 after this PR get merged.

@sequencer sequencer requested a review from a team as a code owner January 15, 2021 17:55
@sequencer
Copy link
Member Author

sequencer commented Jan 15, 2021

request @chick and @jackkoenig to review.

@chick
Copy link
Contributor

chick commented Jan 27, 2021

@sequencer
This is looking pretty close.
I have tried building this against current Z.5 master versions

repo branch hash
firrtl master aec9e9e61f9b6775bf313601ec5a44a34f608609
firrtl-interpeter master 937c44d7ca19c3daca8eea9c52cbaff5476c311b
treadle master 0ec32a5112a2cf0b9741790cf8c8ebb4b8794ddf
chisel3 remove_deprecations 83912479f37247feb209ef362cc6c872b86ca0e5
chisel-testers sequencer:remove_deprecation d4774c6

chisel3 built and passed tests.
chisel-testers did not build. I made the following changes to make it compile and most tests.
The changes mostly involved scala 2.13 and associated val io. My fixes were hacking in particular for the dut.io usages I had to change. @jackkoenig probably can recommend a better way

diff --git a/src/main/scala/chisel3/iotesters/Exerciser.scala b/src/main/scala/chisel3/iotesters/Exerciser.scala
index 30fe124..9cf2a76 100644
--- a/src/main/scala/chisel3/iotesters/Exerciser.scala
+++ b/src/main/scala/chisel3/iotesters/Exerciser.scala
@@ -3,6 +3,7 @@
 package chisel3.iotesters

 import chisel3._
+import chisel3.experimental.DataMirror
 import chisel3.testers.BasicTester

 /**
@@ -48,8 +49,12 @@ abstract class Exerciser extends BasicTester {
     }
   }
   def buildState(name: String = s"$current_states")(stop_condition : StopCondition)(generator: () => Unit): Unit = {
-    //noinspection ScalaStyle
-    device_under_test.io := DontCare  // Support invalidate API.
+    // Support invalidate API.
+    DataMirror.modulePorts(device_under_test).foreach {
+      case (_, _: Clock) =>
+      case (_, port) => port := DontCare
+    }
+
     println(s"building state $current_states $name")
     when(state_number === (current_states).asUInt) {
       when(! state_locked) {
diff --git a/src/main/scala/chisel3/iotesters/IOAccessor.scala b/src/main/scala/chisel3/iotesters/IOAccessor.scala
index 36bdd54..1ae1144 100644
--- a/src/main/scala/chisel3/iotesters/IOAccessor.scala
+++ b/src/main/scala/chisel3/iotesters/IOAccessor.scala
@@ -6,6 +6,7 @@ import chisel3._
 import chisel3.util._
 import chisel3.experimental._

+import scala.collection.immutable.ListMap
 import scala.collection.mutable
 import scala.util.matching.Regex

@@ -13,7 +14,7 @@ import scala.util.matching.Regex
  * named access and type information about the IO bundle of a module
  * used for building testing harnesses
  */
-class IOAccessor(val device_io: Record, verbose: Boolean = true) {
+class IOAccessor(val device_under_test: Module, verbose: Boolean = true) {
   val ports_referenced = new mutable.HashSet[Data]

   val dut_inputs                 = new mutable.HashSet[Data]()
@@ -80,7 +81,13 @@ class IOAccessor(val device_io: Record, verbose: Boolean = true) {
       }
     }

-    parseBundle(device_io)
+    val record = new Record {
+      override val elements: ListMap[String, Data] = ListMap(DataMirror.fullModulePorts(device_under_test):_*)
+
+      //TODO: There must be a better way, but it works ok, I don't think this will be cloned
+      override def cloneType: this.type = this
+    }
+    parseBundle(record)
     port_to_name_accumulator
   }
   val name_to_port = port_to_name.map(_.swap)
diff --git a/src/main/scala/chisel3/iotesters/OrderedDecoupledHWIOTester.scala b/src/main/scala/chisel3/iotesters/OrderedDecoupledHWIOTester.scala
index 7f81981..c20f983 100644
--- a/src/main/scala/chisel3/iotesters/OrderedDecoupledHWIOTester.scala
+++ b/src/main/scala/chisel3/iotesters/OrderedDecoupledHWIOTester.scala
@@ -391,7 +391,7 @@ abstract class OrderedDecoupledHWIOTester extends HWIOTester {
    * by either a decoupled or valid
    */
   override def finish(): Unit = {
-    io_info = new IOAccessor(device_under_test.io)
+    io_info = new IOAccessor(device_under_test)

     processInputEvents()
     processOutputEvents()
diff --git a/src/main/scala/chisel3/iotesters/SteppedHWIOTester.scala b/src/main/scala/chisel3/iotesters/SteppedHWIOTester.scala
index a2bba3d..33869d3 100644
--- a/src/main/scala/chisel3/iotesters/SteppedHWIOTester.scala
+++ b/src/main/scala/chisel3/iotesters/SteppedHWIOTester.scala
@@ -170,8 +170,11 @@ abstract class SteppedHWIOTester extends HWIOTester {
   }

   override def finish(): Unit = {
-    io_info = new IOAccessor(device_under_test.io)
-    device_under_test.io := DontCare
+    DataMirror.modulePorts(device_under_test).foreach {
+      case (_, _: Clock) =>
+      case (_, port) => port := DontCare
+    }
+    io_info = new IOAccessor(device_under_test)

     processEvents()

The following tests did not pass. All due, I think, to similar problems with val io not being accessible

[error] Failed: Total 109, Failed 4, Errors 0, Passed 105, Ignored 1, Canceled 2
[error] Failed tests:
[error] 	examples.AccumulatorBlackBoxPeekPokeTest
[error] 	chisel3.iotesters.BlackBoxVerilogDeliverySpec
[error] 	examples.AccumulatorBlackBoxPeekPokeTestVerilator
[error] (Test / test) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 80 s (01:20), completed Jan 27, 2021 7:27:50 PM

@sequencer
Copy link
Member Author

Oh, sorry, I only tested 2.12, I'll take a look on those tomorrow.

@sequencer
Copy link
Member Author

These issues are caused by chipsalliance/chisel#1744, it was merged after my PR.
3123cc6 fixed for this by adding suggestName to val io, this should fix this issue.

@sequencer
Copy link
Member Author

Any updates? @jackkoenig @chick

@jackkoenig
Copy link
Collaborator

Sorry for the delay, yesterday we were getting 3.4.2 out (now published). Will take a look tomorrow.

Copy link
Contributor

@chick chick left a comment

Choose a reason for hiding this comment

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

Looks good to me.
ucb-bar/dsptools#222 will deal with consequences of this to dsptools

Copy link
Collaborator

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

:shipit:

@jackkoenig jackkoenig merged commit b6a58dd into freechipsproject:master Feb 4, 2021
chick added a commit to ucb-bar/dsptools that referenced this pull request Apr 19, 2021
This compbined PR #222 AND PR #222

Depends on freechipsproject/chisel-testers#309
Depends on chipsalliance/chisel#1730

Black boxes required suggest Name to work
setResource on black box is not addResource
Lots of import cleanup
Saturate did it's own firrtl compiling, modified to use current idioms
Changed dsptools Driver to use jiuyangs iotesters DriverCompatibility stuff
Cleaned up dependencies in build.sbt
Removed BuildInfo plugin from build.sbt (I'm not that sure about it, but it was causing build errors)
--
Now uses github actions for CI
chick added a commit to ucb-bar/dsptools that referenced this pull request Apr 19, 2021
* Black boxes required suggest Name to work
setResource on black box is not addResource
Lots of import cleanup
Saturate did it's own firrtl compiling, modified to use current idioms
Changed dsptools `Driver` to use jiuyangs iotesters DriverCompatibility stuff
Cleaned up dependencies in build.sbt
Removed BuildInfo plugin from build.sbt (I'm not that sure about it, but it was causing build errors)

* Add support for github actions to do ci and publishing

* Add support for github actions to do ci and publishing
Added CODEOWNERS file for symmetry with chiseltest which was used for this template
Removed mill as it is not currently supported here

* Update plugins to have ci-release, used for publishing

* fix iotesters version

* bump copyright to force retest

* Move to github actions for ci, update for 3.5 compatabilty
This compbined PR #222 AND PR #222

Depends on freechipsproject/chisel-testers#309
Depends on chipsalliance/chisel#1730

Black boxes required suggest Name to work
setResource on black box is not addResource
Lots of import cleanup
Saturate did it's own firrtl compiling, modified to use current idioms
Changed dsptools Driver to use jiuyangs iotesters DriverCompatibility stuff
Cleaned up dependencies in build.sbt
Removed BuildInfo plugin from build.sbt (I'm not that sure about it, but it was causing build errors)
--
Now uses github actions for CI

* Only build for scala 2.12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants