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

[core] Support breakpoints #354

Merged
merged 2 commits into from
Aug 5, 2019
Merged

[core] Support breakpoints #354

merged 2 commits into from
Aug 5, 2019

Conversation

jerryz123
Copy link
Contributor

Related issue: Resolves #16

Type of change: new feature

Impact: new rtl

Development Phase: implementation

Release Notes

Breakpoints now supported (I think). We can use Rocket's DefaultTestSuite now.

@jerryz123
Copy link
Contributor Author

I should probably double-check this doesn't break buildroot/Fedora.

bkorpan
bkorpan reviewed Jul 25, 2019
View reviewed changes
Copy link
Contributor

@bkorpan bkorpan left a comment

Choose a reason for hiding this comment

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

Potential for some QoR impact, but probably alright. I'm not a fan of instantiating a BreakpointUnit in every pipeline slot of the fetch buffer enqueue logic, it seems like this could be done more efficiently, though it might be nasty code.

src/main/scala/ifu/fetch-control-unit.scala Outdated Show resolved Hide resolved
@jerryz123 jerryz123 force-pushed the breakpoints branch 2 times, most recently from c7a4fee to aa674fe Compare July 29, 2019 19:21
@jerryz123
Copy link
Contributor Author

I defaulted nBreakpoints to 0, which should make the BreakPointUnits dummy modules that always output false.
When this gets merged, I'll open an issue that mentions updating the frontend breakpointunit.

@jerryz123 jerryz123 requested a review from bkorpan July 29, 2019 19:31
abejgonzalez
abejgonzalez approved these changes Jul 31, 2019
View reviewed changes
Copy link
Contributor

@abejgonzalez abejgonzalez left a comment

Choose a reason for hiding this comment

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

LGTM as long as the default behavior is BP = 0 for now.

@abejgonzalez
Copy link
Contributor

Lets wait on this PR until the Chipyard PR gets merged in, that will remove the conflict on the BoomTestSuites file

@jerryz123
Copy link
Contributor Author

Fine with me

@abejgonzalez
Copy link
Contributor

Why does this still conflict? Did the rebase work correctly?

@jerryz123
Copy link
Contributor Author

Why does this still conflict? Did the rebase work correctly?

Looks like I messed up. I'll rebase again.

@jerryz123 jerryz123 merged commit 4e9d496 into master Aug 5, 2019
@jerryz123 jerryz123 deleted the breakpoints branch August 6, 2019 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abejgonzalez abejgonzalez abejgonzalez approved these changes

@bkorpan bkorpan bkorpan approved these changes

Assignees

@jerryz123 jerryz123

Labels
missing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BOOM lacks breakpoint support
3 participants
@jerryz123 @abejgonzalez @bkorpan