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

Coordinate Top and Harness generation #168

Merged
merged 2 commits into from
Jul 31, 2019
Merged

Conversation

albert-magyar
Copy link
Contributor

This avoids name conflicts between the design top and the test harness by coordinating the two FIRRTL compiler runs into one invocation of the main generator app. It also makes Verilog simulator generation somewhat faster.

Coordinated with ucb-bar/barstools#63

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. Tested. We need to get barstools bumped. I also think we should at least another approval on the barstools PR.

Copy link
Contributor

@jwright6323 jwright6323 left a comment

Choose a reason for hiding this comment

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

I'm not sure I'm convinced that the harness blackboxes are handled correctly here, so I think we need to try this out on a design with a harness blackbox before merging. TOP_TARGETS and HARNESS_TARGETS are a nice touch.

common.mk Outdated Show resolved Hide resolved
variables.mk Outdated Show resolved Hide resolved
@abejgonzalez
Copy link
Contributor

Also ran this with the systolic project that initially caused the issue and this seemed to work.

@abejgonzalez
Copy link
Contributor

Adding the "DO NOT MERGE" until the harness/top .f discussion is completed.

@abejgonzalez
Copy link
Contributor

If I remember correctly... bumping FIRRTL cause a bunch of BOOM "firrtlation" issues where we would run out of memory. I don't know if this is a similar issue to the memoization problem that BOOM encountered in the past.

@colinschmidt
Copy link
Contributor

If I remember correctly... bumping FIRRTL cause a bunch of BOOM "firrtlation" issues where we would run out of memory. I don't know if this is a similar issue to the memoization problem that BOOM encountered in the past.

Do you mean the firrtl issue I worked around here: ucb-bar/hwacha@ff4605f

@abejgonzalez
Copy link
Contributor

Yup. That looks about right. To be honest, I wasn't familiar with the issue since Albert/David solved the issue before the BOOM team saw it.

@albert-magyar
Copy link
Contributor Author

So this seems like just a "bump FIRRTL to head of master" issue. I can reproduce the OOM issues locally, but if I cherry-pick the FIRRTL commit that adds my .f features, things build fine.

I need to bisect this.

@abejgonzalez
Copy link
Contributor

Can we merge dev into this to rerun CI with the new FIRRTL/barstool changes? (You probably also need to combine the barstools branches also)

@abejgonzalez
Copy link
Contributor

This LGTM. I would prefer that the barstools PR is merged in first though.

@albert-magyar
Copy link
Contributor Author

So the barstools PR is merged and this is passing all CI checks now.

Copy link
Contributor

@jwright6323 jwright6323 left a comment

Choose a reason for hiding this comment

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

LGTM

@albert-magyar albert-magyar merged commit c487ca2 into dev Jul 31, 2019
@abejgonzalez abejgonzalez mentioned this pull request Aug 30, 2019
@albert-magyar albert-magyar deleted the avoid-extmodule-conflicts branch September 26, 2019 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants