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.

Sign up for GitHub

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

Jump to bottom

Tests for RWTapping aggregates #3443

Merged
merged 7 commits into from
Aug 8, 2023
Merged

Tests for RWTapping aggregates #3443

merged 7 commits into from
Aug 8, 2023

Conversation

debs-sifive
Copy link
Contributor

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • Internal or build-related (includes code refactoring/cleanup)

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.5.x, 3.6.x, or 5.x depending on impact, API modification or big change: 6.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@debs-sifive
Copy link
Contributor Author

Draft PR b/c some of the firrtl generated is hitting a bug in firtool (which @dtzSiFive is working on). Not sure if we want to hold off on merging these tests until the output they emit are fully supported by firtool.

@debs-sifive debs-sifive added the Internal Internal change, does not affect users, will be included in release notes label Jul 27, 2023
@dtzSiFive
Copy link
Member

I believe these should all work as of latest firtool ( as of llvm/circt@8c83a96 ), FWIW!

@jackkoenig
Copy link
Contributor

I think the tests are fine since they are unit tests, although integration tests would also be handy.

jackkoenig
jackkoenig reviewed Aug 2, 2023
View reviewed changes
src/test/scala/chiselTests/BoringUtilsTapSpec.scala
Comment on lines +256 to +257
"define bore = rwprobe(b.x)",
"define bore_1 = rwprobe(b)",
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 love the reliance on a default name coming from BoringUtils, is the use of BoringUtils really necessary or can we test the same behavior by just having the probe IOs on Child?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or put another way, are we trying to test the behavior of BoringUtils here or just basic tapping of aggregate children? It appears to me it's more about the latter and the use of BoringUtils is just making the test a little more concise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are we trying to test the behavior of BoringUtils here or just basic tapping of aggregate children?

Kind of both... the new tap API is implemented in BoringUtils and relies on bores, which is why bores are being created.

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 I missed that this test is in the file BoringUtilsTapSpec, if that's what we're testing, that's what we should be testing 😅

@debs-sifive debs-sifive marked this pull request as ready for review August 7, 2023 22:09
@debs-sifive debs-sifive requested a review from jackkoenig August 8, 2023 16:10
@debs-sifive debs-sifive added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Aug 8, 2023
@mergify mergify bot merged commit 6c7be5d into main Aug 8, 2023
@mergify mergify bot deleted the tap-agg-tests branch August 8, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackkoenig jackkoenig jackkoenig approved these changes

@seldridge seldridge Awaiting requested review from seldridge

@azidar azidar Awaiting requested review from azidar

Assignees
No one assigned
Labels
Internal Internal change, does not affect users, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@debs-sifive @dtzSiFive @jackkoenig