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

[question] Clarification on Guarantees for High/Mid/Low FIRRTL #674

Closed
seldridge opened this issue Oct 2, 2017 · 7 comments
Closed

[question] Clarification on Guarantees for High/Mid/Low FIRRTL #674

seldridge opened this issue Oct 2, 2017 · 7 comments
Milestone

Comments

@seldridge
Copy link
Member

seldridge commented Oct 2, 2017

I had an odd realization in that FIRRTL levels below High do not respect last connect semantics. The ExpandWhens transform is expected to clean this up for you.

A result of this is that transforms that add new statements (that are not "in place" modifications of existing statements) should either take place at the High FIRRTL level or emit High FIRRTL so that the circuit will be cleaned up for you.

Are there any existing clarifications of what guarantees are expected for specific transforms at specific FIRRTL levels?

  • Technically these guarantees can be gleaned by reading the LoweringCompilers code...

What I've discovered:

  • circuits <= Mid FIRRTL do not obey last connect semantics (cleand up by ExpandWhens)
  • circuits == Mid FIRRTL provide no guarantees that signal names will not alias to each other (cleaned up by Uniquify), e.g., adding the signal wire r_x: UInt<5> to a module with existing signal wire r : {x: UInt<5>} will result in a hidden name conflict

These may be bugs or they may be a violation of the [High, Mid, Low] FIRRTL contract.

Alternatively, this may motivate a new CheckMidForm pass (to save me from myself...).

Edit: After digging a bit more, this is some documentation of this in the code: Compilers.scala. I think that the restrictions are stricter, nonetheless.

@jackkoenig
Copy link
Contributor

Yeah this is a big area for documentation and implementation improvement. The best place to learn about guarantees for the various Firrtl forms is actually to look at the ScalaDoc for the various forms in Compiler.scala: https://github.com/freechipsproject/firrtl/blob/1b8bd0a8d3a0706c3f4d77aef16817163c1e8bfd/src/main/scala/firrtl/Compiler.scala#L117
@azidar, we really need to lay this out better in the spec and probably in a wiki page.

Furthermore, we definitely need CheckMidForm and CheckLowForm as well as a canonical way to run them with your custom pass (possibly a command-line option and accompanying Annotation) to run additional checks after custom passes). Similar additional checking functionality needs to be part of our tests as well.

@edwardcwang
Copy link
Contributor

Also on https://chisel.eecs.berkeley.edu/api-firrtl/#firrtl.CircuitForm now (may need to expand "Known Subclasses").

@jackkoenig
Copy link
Contributor

As far as I can remember, there is also no formally laid out definition of when Uniquification should be run, but off the top of my head it looks like it should be part of lowering from Mid -> Low since that's where the ambiguity would be introduced, but due to such ambiguity originally being illegal, we run it much earlier and thus if you write a pass that consumes Low and generates High, we aren't rerunning it for you. @azidar, we need to formalize this a bit better.

@jackkoenig
Copy link
Contributor

Oh cool, thanks @edwardcwang, I didn't realize we were publishing the ScalaDoc

@seldridge
Copy link
Member Author

@jackkoenig: For Uniquification, it wasn't clear to me if this is something that should be run after a transform directly (how I was originally doing it) or if Uniquification is a requirement of Mid FIRRTL and lower.

Effectively, is it a requirement of Mid FIRRTL and lower that types which would conflict via flattening are the same signal or are different signals?

It would seem logical that these are different signals, otherwise _ has a special meaning in Mid FIRRTL that is equivalent to sub referencing via the . operator. Alternatively, I can also see it being a requirement that names do not conflict in Mid FIRRTL and that it's the responsibility of the user to uniquify to remove conflicts.

@edwardcwang: Thanks! I similarly was not aware that the API was being published... saves me opening it up locally!

@jackkoenig
Copy link
Contributor

@seldridge I agree with you're interpretation that signals that would conflict via flattening are different signals, but clearly the compiler is just not really enforcing this at the moment. We either, need to run Uniquification as part of the MiddleToLow transform or we need to error if a collision would occur. My opinion is that we should do the former, ie. run uniquification during middle to low.

For the time being, you should run uniquification, but @azidar we should make this unnecessary.

@azidar azidar added this to the 1.2.0 milestone Mar 9, 2018
@seldridge
Copy link
Member Author

The guarantees for this are explained in the comments. The Dependency API refactor will make it explicit what is required and what is invalidated. See: #948.

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

5 participants