Skip to content
This repository has been archived by the owner on Apr 20, 2024. It is now read-only.

Remove large annotations #66

Merged
merged 2 commits into from
Nov 6, 2019
Merged

Remove large annotations #66

merged 2 commits into from
Nov 6, 2019

Conversation

abejgonzalez
Copy link
Contributor

This PR removes large annotations that were deleted but kept inside of the output json. This fixes an OOM error on beagle, but I suspect it will happen on other very large designs.

Do we need any of these annotations for other tooling that happens after the Generate... steps?

@seldridge
Copy link
Member

seldridge commented Aug 17, 2019

I think you only need these annotations if you are using FIRRTL or Chisel Drivers. Those need to construct execution results and that logic has to kludgily rely on deleted annotations. If you are fully on ChiselStage and FirrtlStage it should be fine to delete these.

It's been a while, but I thought there were some options to either remove these automatically or optionally remove these.

Working from memory here, so this info may be wrong.

Edit: totally okay to remove DeletedAnnotations from emitted JSON.

@albert-magyar
Copy link
Contributor

@seldridge, I think this is just filtering them for the manual dumping of annotations to a JSON file that happens in the barstools flow. Would just dropping all DeletedAnnotations be okay?

@seldridge
Copy link
Member

@albert-magyar: dropping all DeletedAnnotations from the output JSON is totally fine. In fact, all Stages will do this automatically for you (unless you disable this behavior with WriteDeletedAnnotation/--write-deleted). Ignore my comments above...

@abejgonzalez
Copy link
Contributor Author

Thanks for the clarification @seldridge and for the PR fix @albert-magyar! This LGTM. @colinschmidt @jwright6323 do we care about travis passing?

Copy link
Contributor

@albert-magyar albert-magyar left a comment

Choose a reason for hiding this comment

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

LGTM

@colinschmidt
Copy link
Contributor

Yes we care about travis passing generally.

@abejgonzalez
Copy link
Contributor Author

@colinschmidt @jwright6323 Is this good to go? If so, Ill go ahead and merge.

Copy link
Collaborator

@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.

Fine w/ me

@jwright6323
Copy link
Collaborator

@abejgonzalez we should fix CI in a separate PR, though

@abejgonzalez abejgonzalez merged commit 4db4ebb into master Nov 6, 2019
@abejgonzalez abejgonzalez deleted the large-anno-remove branch November 6, 2019 01:16
@abejgonzalez abejgonzalez mentioned this pull request Nov 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants