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

Change BundleLiteral to RecordLiteral #1400

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

sequencer
Copy link
Member

Related issue:

Type of change: feature request

Impact: API addition (no impact on existing code)

Development Phase: proposal

Release Notes

Since RocketChip use Record for AutoBunlde and diplomatic bundles like TLBundle, if we need to test RocketChip with native chisel tester, we should allow Lit to Record.
This PR changes all BundleLiterals to RecordLiterals, which enables Lit method for Record, and deprecate original BundleLiterals, it won't affect existing code, and enalbe peek/poke for Diplomatic Bundle in chiseltester.

@sequencer sequencer requested a review from a team as a code owner April 4, 2020 15:25
@ekiwi ekiwi requested a review from ducky64 April 4, 2020 17:01
Copy link
Contributor

@ducky64 ducky64 left a comment

Choose a reason for hiding this comment

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

The implementation looks fine.

I'm not sure that I want to recommend the use of RecordLiterals as a name, since much fewer users will be familiar with Record rather than Bundle (despite it technically being more correct). That being said, when we mainline bundle literals, there will no longer be a need to import a BundleLiterals object at all.

@sequencer
Copy link
Member Author

Yes my intention is to PR a minimal patch to this feature.
I'm not sure if I we can remove RecordLiterals/BundleLiterals, and directly use implicit def for this feature, since it contains a breaking modification to codes.

@ducky64
Copy link
Contributor

ducky64 commented Apr 6, 2020

We discussed this at the meeting today and:

  • agree that the feature (providing a Record literals interface) should be implemented
  • are concerned that the rename will break binary compatibility
  • because BundleLiterals is an experimental import (which hopefully will be mainlined someday and obsoleted) and Record is an advanced user construct, the best solution (though technically "incorrect") is probably just to expand the BundleLiterals API to also take Records. So, basically, what you have now minus the BundleLiterals -> RecordLiterals rename.

@sequencer
Copy link
Member Author

I think this is a minimal patch now:)

Copy link
Contributor

@ducky64 ducky64 left a comment

Choose a reason for hiding this comment

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

lgtm

@ducky64 ducky64 merged commit cd2bdb2 into chipsalliance:master Apr 6, 2020
@sequencer sequencer deleted the record_lit branch April 7, 2020 04:19
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.

2 participants