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

Prune/Don't Emit Output Annotation Sequence #1272

Closed
5 tasks done
seldridge opened this issue Dec 4, 2019 · 14 comments · Fixed by #1277 or #1278
Closed
5 tasks done

Prune/Don't Emit Output Annotation Sequence #1272

seldridge opened this issue Dec 4, 2019 · 14 comments · Fixed by #1277 or #1278

Comments

@seldridge
Copy link
Member

Checklist

  • Did you specify the current behavior?
  • Did you specify the expected behavior?
  • Did you provide a code example showing the problem?
  • Did you describe your environment?
  • Did you specify relevant external information?

What is the current behavior?

There are reported issues related to Stages always serializing their output annotation sequence. Either there needs to be a way to turn this off or too many or too many large annotations are being serialized. It may also be worth not serializing annotations, unless the user requests it. This is really a method of checkpoint-restart or an interface between stages that are communicating via files.

The result of this is out-of-memory errors in the JVM during annotation serialization.

This may be as simple as using a DontSerialize annotation that is attached to large things which are serialized in other ways (e.g., circuits).

What is the expected behavior?

FIRRTL shouldn't get out-of-memory errors for normal circuits (Rocket Chip sized?).

Steps to Reproduce

Unclear at this time. @oharboe may be able to help.

Your environment

  • Chisel 3.2.0
  • FIRRTL 1.2.0.

External Information

https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/chisel-users/qbx_G58dCEY/eJN-IfOzAwAJ

@oharboe
Copy link

oharboe commented Dec 4, 2019

I can't provide the source code as this really only appears to be happening in a larger context.

I'll try to describe the code involved in general terms...

  1. The minimum that will reproduce it three levels of modules. Our design is deeper, but I will call the three levels involved: Top, Medium, Low.
  2. There's 1 Top module, 1 medium used by the top level module, and ~64 low level modules used by the medium module. The top module also uses various other modules.

The Top module is not particularly big, even with all the sub-modules. It's ca. 20000 ALMs as measured in Altera Quartus speak, which is ~5% of the biggest Arria 10 FPGA (28 nm). I guess I could even say that it's pretty small as things go these days.

To hazard a guess, I believe that there's a problem with some sort of global optimization/analysis in Chisel that is blowing up.

The Altera P&R tools have no problem with this design. Verilator compiles it in no time.

That said, before I reworked the code, I had a single mid-level module that incorporated the low level module. This giant mid-level module caused Verilator to generate .cpp files that g++ took forever to build.

@seldridge
Copy link
Member Author

I haven't had time to really dig into this, but I noticed that the FirrtlCircuitAnnotation is hanging around and getting serialized to JSON. I'm 90% sure this is what the problem is here. The notion of a DontSerializeAnnotation has been discussed and this is likely the primary use case for it.

Namely, things which are serialized to files should not be included in the serialized output annotation sequence. However, for inter-stage communication not through files it's fine to leave the full annotation sequence intact.

There's some initial work on this here: https://github.com/freechipsproject/firrtl/tree/dont-serialize

@seldridge
Copy link
Member Author

Concretely the issue is that the FirrtlCircuitAnnotation is being serialized into the output annotation JSON file. An output annotation file will only be generated if you provide the -foaf/--output-annotation-file/OutputAnnotationFileAnnotation to a stage. However, Chisel has an AddImplicitOutputAnnotationFile phase which will add this annotation if one is not provided (or provided via the short/long option).

This problem didn't come up for Rocket Chip because that project is only serializing annotations internal to the circuit and not these annotations that come from options. Therefore, this implicit output annotation file option was never communicated.

#1269 should fix this.

@seldridge
Copy link
Member Author

@oharboe: This should be fixed via #1278 which will show up in 1.2.2.

@oharboe
Copy link

oharboe commented Dec 17, 2019

@seldridge Does a unit-test exist to check for this problem?

@oharboe
Copy link

oharboe commented Dec 17, 2019

@seldridge I tried to test with 0.1-SNAPSHOT, but it fails to build with the following error message:

Symbol 'type chisel3.experimental.MultiIOModule' is missing from the classpath.

@seldridge
Copy link
Member Author

@oharboe: There's actually not a unit test for this problem. Granted, I think the unit test would really be that Unserializable isn't being written.

That's an error that's indicative of something else... There was a flattening of the package hierarchy that moved chisel3.experimental.MultiIOModule into chisel3.MultiIOModule.

You should be able to just add a library dependency for testers2 as 0.1-SNAPSHOT and things work. However, that may require publishing all the other things locally and in-order. (FIRRTL, then Chisel, then {Treadle, Interpreter}, then {IOTesters, Teters2}).

@oharboe
Copy link

oharboe commented Dec 17, 2019

@oharboe It's not practical to build and publish locally as I'm testing on a build server, which is where it crashed. What would be the correct package combo to try out?

@seldridge
Copy link
Member Author

You should be able to use master of all packages and things will work.

@oharboe
Copy link

oharboe commented Dec 17, 2019

Gotit. However, it's not practical to build Chisel on our build server, so I need prepublished packages. Is there a publish package combo I can test with?

@ucbjrl
Copy link
Contributor

ucbjrl commented Dec 17, 2019

The most recently published SNAPSHOT of chisel-testers2 is v0.2-20191127-SNAPSHOT. This should pull compatible versions of all the other libraries.

@oharboe
Copy link

oharboe commented Dec 17, 2019

@ucbjrl quite. that image doesnt include the fix, so i guess i wait.

@ucbjrl
Copy link
Contributor

ucbjrl commented Dec 20, 2019

We've just published 0.2-20191218-SNAPSHOT of chisel-testers2 which should depend on the v1.3-20191218-SNAPSHOT version of FIRRTL which should contain this fix. This fix has also been back ported to FIRRTL v1.2.2 which should be pulled in by chisel-testers2 v0.1.2.

Note: the 0.2.x branch of chisel-testers2 has undergone a package rename - from chisel-testers2 to chiseltest

@oharboe
Copy link

oharboe commented Dec 20, 2019

@ucbjrl I've upgraded to chisel-testers2 v0.1.2 and removed my hack and now it works. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants