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

[feature request] adding prefix to coreir/commonlib/... verilog modules #990

Closed
leonardt opened this issue Mar 1, 2021 · 4 comments
Closed
Assignees

Comments

@leonardt
Copy link
Collaborator

leonardt commented Mar 1, 2021

The desired use case is generating a design where all the relevant verilog modules have a specific prefix (this is used to distinguish the design components from others when integrated in a larger design with blocks from multiple sources). Right now we can use the namespace feature to insert a prefix before user defined modules, but we can't insert this prefix for coreir libraries (e.g. primitives, commonlib). Mechanically this should be easy enough to support in the backend, but how can we expose this as an option to the user? One simple option might be to have some metadata field (e.g. verilog prefix), then at the magma level we could insert these for all the non user namespace modules referenced.

@rsetaluri
Copy link
Collaborator

I think the best option is to have an unsafe output_name field on any CoreIR module (maybe instance too), which will override the default CoreIR name generation logic. Throwing a bunch of logic into name generation @ CoreIR level seems complicated and hard to get right. We could do logic at the higher levels (e.g. magma) to make sure that any output_name overrides are safe.

@leonardt
Copy link
Collaborator Author

leonardt commented Mar 2, 2021

A seemingly simple option: provide some option for verilog code generation for a prefix that goes before all modules (e.g. coreir --verilog_module_name_prefix=foo). Then, simply do a pass on all the verilogAST modules and insert the prefix. The main benefit here is that this should work for code that includes multiple namespaces (e.g. we may want to have separate namespaces to handle shared names, but at the end of the day we want everything to have a shared prefix for the above use case). This feels simpler than having to do a pass at the magma level to set output_name for all modules. Also, I'm not sure that would work for commonlib muxes for example, since in magma we reference them as instances of generators, to the actual generated module does not have a handle until rungenerators is invoked later on (so we'd need to do this logic at the coreir level not the magma level)

@rsetaluri
Copy link
Collaborator

A code-generation/verilogAST specific pass seems fine to me. I was concerned if it was a CoreIR pass/abstraction, it would be clunky but doing that in verilogAST seems reasonable.

@rdaly525
Copy link
Owner

rdaly525 commented Mar 3, 2021

The verilogAST pass seems reasonable to me.

leonardt added a commit that referenced this issue Mar 8, 2021
* [#990] Add compile option for verilog prefix

* Add binary interface

* Add instance logic

* Track only changed modules

* Add complex test

* Add missing files

* Encapsulate addPrefix logic, include in writeToFiles flow
@leonardt leonardt closed this as completed Mar 9, 2021
rdaly525 added a commit that referenced this issue Apr 21, 2021
* Fix CORENamedTypeToString c-API (#982)

This change adds a context-managed scratch-pad memory which can be used
safely, without risk of a memory leak. The c-API CORENamedTypeToString
previously returned a pointer to a temporary, which is unsafe. Instead,
we allocate context-managed memory and return a buffer contained there.

See #969.

* Update C-API for record to use deterministic order (#985)

Possible fix for
phanrahan/magma#920 (comment)

Before, this was iterating over a std::map which is not ordered.  This
changes it to use the `getFields` API which should return the same order
for the fields.

* bumps version

* Add compile guard support (#986)

* Add compile guard support

* Bump verilogast commit

* Simplify test

* Add support for verilog body string (#987)

This was a feature we used to support in magma (see first example
https://github.com/phanrahan/magmathon/blob/master/notebooks/advanced/verilog.ipynb)
that used the verilog backend.  This adds support for the same feature
in the coreir backend.

This simply allows the user to define circuit interface in magma/coreir,
but the body in verilog.  This is slightly different than inline verilog
in that we allow the user to drive signals from the verilog body string.
Otherwise, if we drove a signal from inline_verilog, we'd get an
undriven error in magma/coreir since we aren't aware of the driver from
the string.

* Support user_namespace with verilog_body (#989)

* Add support for inverted compile guard (#988)

* Add support for inverted compile guard

Support for the `ifndef` pattern has been requested.

* use type=defined/undefined instead of invert

* Clang format

* [#990] Add compile option for verilog prefix (#991)

* [#990] Add compile option for verilog prefix

* Add binary interface

* Add instance logic

* Track only changed modules

* Add complex test

* Add missing files

* Encapsulate addPrefix logic, include in writeToFiles flow

* Adds support for outputs inside a compile guard (#992)

By default, we drive outputs to be 0 inside the else clause (should work
for arbitrary ports since types have been flattened).

This allows for "stubs" to be inserted when a compile_guard is not
active.

This feature is simpler than supporting a general output handler in the
compile guard, since then we'd need to support multiple drivers from
different compile guard cases which is tricky since in magma/coreir we
only allow a single driver for a port.  This covers the use case at FB
without having to dive into the details of general output handling.

* Add option for verilog-prefix of extern modules (#993)

* Add option for verilog-prefix of extern modules

* passthrough arg

* Update src/passes/analysis/verilog.cpp

Co-authored-by: rsetaluri <rsetaluri@users.noreply.github.com>

* Update src/passes/analysis/verilog.cpp

Co-authored-by: rsetaluri <rsetaluri@users.noreply.github.com>

* Fix metadata override case

Co-authored-by: rsetaluri <rsetaluri@users.noreply.github.com>

* Add symbol table shell/scaffolding (#984)

* Add symbol table shell/scaffolding

* Symbol table updates

Adds debug option to pass manager (and coreir binary). Creates concrete
implementation of symbol table API. Updates flattentypes pass to use
symbol table API

* Move symbol table impl into separate file

* Add dummy implementation for CoreIRSymbolTable

* Add json interface

* Implement basic symbol table data structures

* Implement jsonification routine

* Update port name interface

This change removes the output module name from the set/getPortName
interface.

* Write symbol table to file

* Implement simple instance name mapping

* Add inline instance API

This change just adds the APIs and nec. data structures for keeping
track of inlining.

* Add TODO for addPrefix re: symbol tables

* Fix Jsonifier template param

Co-authored-by: Ross <rdaly525@stanford.edu>

* bumps version

* Link (#996)

* Refactored json serialization code. added a serialization pass based off
an instancegraph pass. Deprecated saveToFile. Added loadHeader API and
linkImpl API. added a couple tests

* fixes serialization format

* adds headergen and implgen tests

* impl generation now works

* header gen tests work

* fixes base path for linking test

* un-deprecated saveToFile

* FileReader fixes. Adds a unit test in gtest with the anytest and saveAndLoad

* test fixes to prevent impl link error

* renames impl to definition. renames some helper functions for clarification

* clarifying comments

* Instrument inline symbol table (#997)

* Add back flattentypes test

In a force push, commit 0e1b09e was
lost. This commit adds it back.

Original commit message:

"adds basic symbol table test for flatten types"

* Add simple inlineInstance test

* Move set-up and tear-down to test fixture

* Add inlineInstance symbol table test

NOTE: This is just the test for inlining. It does not yet instrument
the pass to update the symbol table.

* Support multi-level inlining

This commit adds support to the SymbolTableInterface API to allow for
multi-level inlining. It does this by making the return type of
get_inlined_instance_name() to be Union[str, Sentinel] (just like
get_instance_name()). We also update the internal data structures to
support storing this union, as well as ser/deser.

NOTE: We still have not instrumented the inline pass.

* Add instance type API

Add API to allow for get/setting types of instances (a simple str ->
str -> str function).

* Update inlined instance API to take a vector of instances

* Fix typo

setInlineInstance -> setInlinedInstance

* Update inline test for new API

* Update simple hierarchy example

* Update inline test for new hierarchy example

* Revert inlinedInstance API change

This change reverts the change to make the {set,get}inlinedInstance API
take in a vector. We now take in a pair as before.

* Add scaffolding for logging

This change introduces logging APIs to the symbol table to allow for
staged symbol table updates.

TODO: Implement the logic for finalize() (merging logs) and pass the
right arguments into the logging call from inlineInstance().

* Fix basic inline test

* Make logger accesible directly from symbol table

* Update logger interface

* Log new instances in module def

* Log inlined instances in inlineInstance()

* Add debugging capabilities to logger

* Skip logging new/removed passthrough instances

* Fix debug iterator bug

* Pause logging during extraneous actions in inlineInstances

* Implement symbol table logging finalization

* Set types during finalization

* Fix compiler issues/warnings

* Fix compiler issues/warnings

* Put symbol table behind flag

* Fix minor issues

* bumps version

* Link capi (#998)

* adds serializer_header api

* adds loadheader api

* updates c api for link apis

* bumps version

* Skip symbol table logging for generators (#999)

* Bump version (#1000)

* Use module longmame for symbol table (#1001)

Using a module's longname allows us to include generators and circuits
across namespaces.

* adds get function for instance metadata (#1002)

* adds get function for instance metadata

* Update src/coreir-c/coreir-c.cpp

Co-authored-by: rsetaluri <rsetaluri@users.noreply.github.com>

* bumps version

* Multiplier variations in commonlib for returning middle and high bits (#1004)

* Mult variations (#1005)

* Multiplier variations in commonlib for returning middle and high bits

* Fixes to multiply variations

* Multiply variations; remove unintended comments

* bumps version

* demangles stack trace (#887)

* Canon (#980)

* adds verify_cannonical pass and starts canonicalize pass

* Adds back API for CoreIRTypes as values

* Adds python test for canonicalizing counters.json

* Adds pack and unpack to coreir namespace

* bug fix

* adds reconnect convenience function

* adds passes

* Update wireable.h

fixes comment

* fixes

* Adds canon src code

* Code cleanup

* Fixes unused var warnings with strucutred bindings

* comment and code clarity fixes

Co-authored-by: Raj Setaluri <raj.setaluri@gmail.com>

* Add unused() function (#983)

Co-authored-by: rsetaluri <rsetaluri@users.noreply.github.com>
Co-authored-by: Leonard (Lenny) Truong <lenny@stanford.edu>
Co-authored-by: Jeff Setter <setter@stanford.edu>
Co-authored-by: Raj Setaluri <raj.setaluri@gmail.com>
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

No branches or pull requests

3 participants