Use "_" for Inline Name Mangling, Respect Namespaces #901
Conversation
There was a problem hiding this comment.
This looks like a clean implementation to me. Tests look like the back up the changes.
I don't feel particularly qualified to opine on the consequence of these changes to the
firrtl/chisel ecosystem. I defer to @seldridge @jackkoenig for that determination
|
In terms of implications:
I expect that all of these are exceedingly rare cases. @chick: Since this breaks Chisel temporarily, what's the correct strategy to merge this? I'm assuming that I merge this and then merge the companion PR once Jenkins pulls in upstream FIRRTL. Jenkins does pull in upstream FIRRTL, yes? While I'm doing this, all FIRRTL PRs will break as they'll be trying to use upstream Chisel. After everything is updated, this will then requires developers to use @jackkoenig: Are you okay with this without prefix uniqueness? I was thinking that would be a follow-up PR once I look at the performance implications, if any, on a large design like Rocket Chip. Pending these questions, I'll intend to get this in and then quickly merge chipsalliance/chisel#898 (which is already approved). I have a follow-up ready to go for FIRRTL that will fix Verilog keyword name mangling. |
|
I believe if you include the string Jenkins does build the FIRRTL master branch as part of its chisel testing, so this may temporarily cause chisel tests to fail (depending on how quickly you can commit the chisel companion PR). This does happen from time to time, so it's not unexpected. We can specify specific branches/PRs/SHA1s for all of the BIG6 repositories when triggering a manual Jenkins build. I would like to do this (before starting the chain of commits to masters) to verify that we do have a consistent global state. |
This adds a method, cloneUnderlying, to Namespace that returns a copy of the underlying mutable.HashSet. This is useful for constructing a Namespace that you would like to manipulate manually without using Namespace's methods to generate temporaries. Signed-off-by: Schuyler Eldridge <[email protected]>
This makes findValidPrefix and enumerateNames both private to FIRRTL (previously, these were private). This enables their use for name generation by other FIRRTL passes/transforms. Signed-off-by: Schuyler Eldridge <[email protected]>
Summary of changes:
- Use "_" as an inlining delimiter instead of "$"
- Makes inlining avoid namespace conflicts
This changes the delimiter used for inlining to "_" instead of "$". This
avoids problems with buggy parsers that may not handle "$" correctly. As
ClockListTransform relies on the explicit use of "$", the delimiter is a
FIRRTL-private val that the ClockListTransform overrides (to the original
"$").
Namespace conflicts could occur previously, but are very rare as users
will almost never use "$" in a name (even though it's allowed by both the
FIRRTL and Verilog specifications). Moving to "_" increases the
probability of namespace conflicts occurring. This adds explicit checking
that inlined names will not introduce namespace conflicts and that
generated names are prefix unique (as defined in the spec).
Note: inlined modules may not have unique prefixes. A test is included
that this is the case and an ignored test shows what prefix uniqueness
would look like.
MISC:
- [skip chisel tests]: Changing the delimiter causes the Chisel
InlineSpec to fail as this explicitly checks for "$".
Signed-off-by: Schuyler Eldridge <[email protected]>
|
@ucbjrl: Great. Thanks for the pointer to The specific PRs that should be tested together with the BIG6 (freechips is starting to sound more and more like a college football conference...) are: These are both rebased onto master, approved, and ready to go. I've tested them locally for compatibility, but have not done a local BIG6 test. A BIG6 test on Jenkins before serial master commits makes sense to me. Good suggestion. Is the manual Jenkins build with branch pointers something that I can initiate (with similar tags that Jenkins will understand), or is that something that you will have to do? |
|
You need to have Jenkins credentials (be able to log in) to run a manual Jenkins build. We don't have an anonymous public interface for this. |
|
Thanks a lot @ucbjrl. I'll take care of the serial merge later today. |
Summary of changes
_as a delimiter. Previously,$was used.Overview
This does not change the underlying algorithm of Inline. The only major change is that a returned flattened instance is converted to a bundle with
Uniquify.enumerateNames. The prefix is then generated for that bundle with respect to the enclosing Module's namespace (usingUniquify.findValidPrefix). Additionally, module changes are tracked via aRenameMapas opposed to assuming that one prefix will work.Note: this does not inline with "prefix unique" names. This is somewhat expensive to do as it requires expanding all names in the namespace out to include their prefixes. There's a discussion of the limitations associated with this via a method that PR #900 introduces in this thread.
Small changes
Uniquifymethods are madeprivate [firrtl]findValidPrefixwants aHashSetas input. I expose this with a new method,cloneUnderlying, added toNamespaceMiscellaneous
$is used)