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

Add basic Chisel2 compatibility sanity checks. #340

Merged
merged 9 commits into from
Jan 27, 2017

Conversation

ucbjrl
Copy link
Contributor

@ucbjrl ucbjrl commented Oct 26, 2016

No description provided.

@ducky64
Copy link
Contributor

ducky64 commented Oct 26, 2016

The first part looks like a direct transformation of the Chisel compatibility package. Is it useful, and what kind of bugs is it expected to catch?
Potentially related: https://docs.google.com/document/d/13k8AsgYdF-2TJx9QIHHvPiuV3JMbsrMLkWmmjdYBLVg/edit

The Chisel2 RISC tester makes sense, but I don't think it's well targeted. What Chisel2 compatibility features in particular is it trying to test? Can we directly test those? I imagine individual tests that exercise the different connection semantics would be more useful, especially because a failure gives you a specific cause instead of "my omnibus processor test is on fire".

@ucbjrl
Copy link
Contributor Author

ucbjrl commented Oct 26, 2016

Good point on the compatibility package transform. I'll pull it.

@ucbjrl
Copy link
Contributor Author

ucbjrl commented Oct 26, 2016

The Chisel2 RISC tester is a "sanity" check, which is the issue this test is trying to address.

Testing individual connection semantics is a good idea. I'll add some.

@jackkoenig jackkoenig added this to the 3.0.0 milestone Dec 14, 2016
@jackkoenig
Copy link
Contributor

@ucbjrl Can you check out why this is failing?

@ucbjrl
Copy link
Contributor Author

ucbjrl commented Dec 14, 2016

It's because of src/main/scala/chisel3/compatibility.scala:273

  require(!nodeType.widthKnown, "Bit width may no longer be specified for enums")

Introduced by Simplify Enum API (#385) edb19a0, and CompatibilitySpec.scala:126

val add_op :: imm_op :: Nil = Enum(Bits(width = 8), 2)

Should we update the test or the new (incompatible) requirement?

@ducky64
Copy link
Contributor

ducky64 commented Dec 15, 2016

I'd argue for changing the test. It makes no sense to specify a width in an enum since you can't control the values, and I don't think it was used outside chisel tests.

@ucbjrl
Copy link
Contributor Author

ucbjrl commented Jan 27, 2017

ping

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.

lgtm

val value: Int = Gen.choose(0, Int.MaxValue).sample.get
val l = UInt(value)
l shouldBe a [UInt]
l shouldBe 'lit
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what this line does, but it's awesome

@jackkoenig jackkoenig merged commit 82ec96f into master Jan 27, 2017
@jackkoenig jackkoenig deleted the chisel2sanitychecks branch January 27, 2017 23:34
mwachs5 pushed a commit that referenced this pull request Dec 29, 2022
Bumps [firrtl](https://github.com/freechipsproject/firrtl) from `580844e` to `d1dab1d`.
- [Release notes](https://github.com/freechipsproject/firrtl/releases)
- [Commits](chipsalliance/firrtl@580844e...d1dab1d)

---
updated-dependencies:
- dependency-name: firrtl
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

None yet

4 participants