search

chipsalliance / chisel

Chisel: A Modern Hardware Design Language
https://www.chisel-lang.org/
Apache License 2.0
3.99k stars 597 forks source link

Consolidate directionality data #440

Closed ducky64 closed 7 years ago

ducky64 commented 7 years ago

Right now, we have directionality data being stored in both Data.firrtlDirection (mutable) and in Bindings. These should be consolidated to one.

The rationale seems to be that the Binding direction is fully resolved at the leaf nodes (which is needed for connect checking in Chisel) while Data.firrtlDirection maintains the hierarchical direction (which is needed for FIRRTL's flip semantics).

Good programming practice says that data should be DRY (don't keep the same information in two places, because they'll inevitably get out of sync / is just generally confusing) and defensive programming promotes the use of immutable data structures (which neither of these are, though Binding has limited mutability though its binding_= setter).

Simple solution: leaf directionality only

One solution is to discard Data.firrtlDirection, and only resolve flippedness at leaf nodes. Looking at emitted .fir files and looking at the implementation of Bundle.toType, it seems the FIRRTL emitter writes out full Bundle types, so one possibility would be to emit directionality data at the leaves only. In Chisel, directionality data could be limited to leaf Bindings only. However, this is arguably less clean since Chisel isn't storing user intent (hierarchical directionality), which may have implications for FIRRTL readability.

Cleaner solution: hierarchical directionality only with parent links

So at a high level, it seems a good solution would store the hierarchical directionality and resolve the leaf node directionality on the fly (so the mutable rebinding can be eliminated, this could be memoized for efficiency). The main problem with the current implementation is that if the leaf node is accessed, we don't have parent links to resolve its directionality. However, Bindings could also be expanded to apply on all Data subtypes (instead of just Element) and contain a parent link as a terminal binding (is having parent links generally useful?). Since we now require all operations to happen on bound nodes, the top-level Binding (Wire, Reg, IO, ...) would recursively add parent links before the node is actually used. As for chisel2 compatibility (which didn't require top-level Wire and IO), it would skip the binding system completely and since hierarchical directionality is maintained everything is fine (does anyone know if Bindings does any checks in chisel2 compatibility / NotStrict mode?).

Thoughts? Are there holes in this proposal?

Side note: Motivation

The motivation behind this was to update the chisel3 directionality semantics, but the mutable data flying around everywhere makes things difficult to reason about. At a high level, the idea would be to strictly separate Input, Output, Flipped, and Undirectioned types, mainly for Bundles so that you can have a Input or Output of an Undirectioned Bundle, but only Flip a mixed-directionality Bundle. And the motivation for that change is to provide more intuitive semantics for the whole chiselCloneType / cloneType mess (essentially, what data is kept in which operation). Yak shaving at its finest!

aswaterman commented 7 years ago

Make sure this doesn't break interoperability of chisel2 and chisel3. It is something we should write more tests against before going down this path.

ducky64 commented 7 years ago

Yeah, it should be possible to maintain chisel2 compatibility with NotStrict compile options, which can skip Binding checks. The essential information for FIRRTL generation is retained.

With the requirement that all synthesizable types are wrapped and bound (IO(...), Wire(...) functions), it's possible to ensure that parent links exist before a node is used (it should be an error to try to perform an operation on a non-synthesizable type). Since we didn't have IO(...) and Wire(...) wrap requirements in Chisel2, it would skip the additional checks enabled by Bindings (which I think is the case right now?).

The Bundle directionality changes (in Motivation, which really is a separate issue) and potential to implement checks for #368 aside, this shouldn't modify what the user sees. Those checks would be ignored in NotStrict mode, though I don't have a good deprecation plan so it may be "fix all your chisel3 code that the compiler tells you is unsafe and no longer supported".

Chisel2 tests: isn't that basically rocket-chip? Chisel2 was much more ad-hoc and I think the main (only?) driver for the compatibility layer is rocket-chip and making sure it continues to compile.

aswaterman commented 7 years ago

Yeah, once the chisel3 API settles down, we plan to do further rocket-chip development in chisel3. But that requires that modules written against the compatibility layer can interoperate with modules written against chisel3. Connecting modules written in chisel2 compatibility mode with modules written in chisel3 is the thing I was advocating writing tests for.

ducky64 commented 7 years ago

@sdtwigg since you designed the bindings system, any opinions on hierarchical directionality data only and using parent links to resolve leaf directionality?

sdtwigg commented 7 years ago

My general thoughts are similar as during the original PR. I acknowledge that it would be nice to have the full directionality; however, the reason it was not done is because it was not necessary for the desired features. I am hesitant to complicate the frontend (and potentially introduce bugs to already working features) for speculative gains (which my understanding is currently only "we might use this information in the backend later").

On the matter of DRY, I do not understand why firrtlDirection exists separate from Binding. It was unnecessary for the initial PR.

ducky64 commented 7 years ago

Another reason would be to expose hierarchical resolved directionality data (i.e. unspecified, input, output, bidirectional, mixed specification), which would be a better API than calling flatten and inspecting the leaf elements (which kind of violates the level of abstraction of a Bundle). This would also allow better node type checks (unbound, directionality-bound, terminally bond) when using nodes as synthesizable types vs. models.

The justification for firrtlDirection was that they need hierarchical directionality data to generate FIRRTL's flip semantics.

sdtwigg commented 7 years ago

Given that that the Data hierarchy is effectively sealed at the 'Record, Vec, Element' level, you can still do a hierarchical walk if you want (Binding.bind and MonoConnect/BiConnect do this). I think it would be better served if there was an actual elucidation of the exact benfits (in order to justify risking externally visible breakages when refactoring what is nominally all internal).

I forsee all Port data having a notion of localDirection (which is not just input, output, none but more expanded: input, output, derived, none, flip.) and leaves having trueDirection (just input or output plus maybe inout). The true direction of a leaf is its local direction flipped for however many flips there are from there to the base. Derived (which may be redundant with input/output) is what the leaves of Output(DecoupledIO(...)) or Output(Valid(...)) become and handles the case where a Bundle is 'structified' by using Output or Input. Leaves can only have some localDirections (input, output, derived) while aggregates can have any.

The general goal was to have all DecoupledIO interfaces actually look similar wrt directions for when doing analysis. (Although it may be just as simple to just look for the two valid variants of an interface-like DecoupledIO....)

I'm also very confused and concerned by referring to flatten as part of the Chisel API. flatten is marked as package-private and thus can only be used internally by Chisel to implement the actual API features. (As an aside, it does return a Seq[Bits], which is slightly peculiar as this excludes non-Bits leaves like Clock, FixedPoint, etc.)

My critique regarding the firrtlDirection is mainly that you wouldn't need it if the connection semantics were defined more sanely. (Basically, you somewhat dug your own grave on that one.)

ucbjrl commented 7 years ago

Right. The problem was maintaining compatibility with existing Chisel3 code and the current firrtl implementation, and probably a misunderstanding of Stephen's suggestion. I would be happier if we could eliminate the dual representation.

I think we all would welcome saner connection semantics.

The following was the thread.​​

[image: UC Berkeley Mail] *Jim Lawson

what the flippin' heck ... 4 messages

Jim Lawson < Wed, Aug 17, 2016 at 9:42 AM

To: Adam Izraelevitz, Jack Koenig Cc: Jonathan Bachrach , Stephen Twigg I've managed to get rocket-chip through a version of Chisel3 with Stephen's more explicit connection semantics - I have to disable/moderate some of the tests with suitable compileOptions and there's one source change required due to what I currently suspect is a Scala compiler bug.

Stephen's code converts bulk connects into the appropriate elemental connects. In doing so, it distributes (applies) flips to the elements of an aggregate, i.e.

output io : {flip imem : {flip ready : UInt<1>, valid : UInt<1>, bits :

{btb : {valid : UInt<1>, ...

becomes:

output io : {imem : {ready : UInt<1>, flip valid : UInt<1>, bits : {btb

: {flip valid : UInt<1>, ...

This is causing structural comparisons to fail in firrtl:

firrtl.passes.CheckTypes$MuxSameType:  @[ibuf.scala 85:21]: [module

IBuf] Must mux between equivalent types.

since the true and false mux expression have different tpe()s:

BundleType(ArrayBuffer(Field(taken,Default,UIntType(IntWidth(1))),

Field(mask,Default,UIntType(IntWidth(2))), Field(bridx,Default,UIntType(IntWidth(1))), Field(target,Default,UIntType(IntWidth(39))), Field(entry,Default,UIntType(IntWidth(6))), Field(bht,Default,BundleType(ArrayBuffer(Field(history,Default,UIntType(IntWidth(7))), Field(value,Default,UIntType(IntWidth(2))))))))

BundleType(ArrayBuffer(Field(taken,Flip,UIntType(IntWidth(1))),

Field(mask,Flip,UIntType(IntWidth(2))), Field(bridx,Flip,UIntType(IntWidth(1))), Field(target,Flip,UIntType(IntWidth(39))), Field(entry,Flip,UIntType(IntWidth(6))), Field(bht,Default,BundleType(ArrayBuffer(Field(history,Flip,UIntType(IntWidth(7))), Field(value,Flip,UIntType(IntWidth(2))))))))

The relevant source lines are:

val io = new Bundle { val imem = Decoupled(new FrontendResp).flip ... class FrontendResp(implicit p: Parameters) extends CoreBundle()(p) { val btb = Valid(new BTBResp) ...

val ibufBTBResp = Reg(new BTBResp) ... io.btb_resp := Mux((ibufBTBHitMask & bufMask).orR, ibufBTBResp, io.imem.bits.btb.bits)

The higher order flip (in imem) is effectively ignored when we don't distribute/apply flips, so the structural comparison matches in the code generated by chisel3 "master".

Am I interpreting this correctly? Don't we need to canonicalize before comparing structures? Should flips even be taken into account when performing a structural comparison?

Stephen, how are you avoiding this? Do you have firrtl changes we should incorporate?

Stephen Twigg Wed, Aug 17, 2016 at 11:08 AM

To: Jim Lawson Cc: Adam Izraelevitz , Jack Koenig , Jonathan Bachrach I review of the FIRRTL spec (circuitously) requires all parts of both input operands to have no part marked as flipped. (This legislates out of having to deal with what a flipped field in a Mux would actually mean).

I think this actually demonstrated a slight regression in all versions of Chisel3 from Chisel2, which allowed you to Mux between two DecoupledIO (with the understanding that the result was read-only). The presence of the ready field violates the passive requirement. If the DecoupledIO are flipped from each other you violate the equivalence issue. The former can happen as a matter of regular (you Mux between two DecoupledIO and then examine a field). The latter seems like it would occur less frequently. "val triggering = Mux(trigger_on_enq, queue.enq, queue.deq).firing"

You can get around this in two ways:

  1. Immediately in my PR: For each operand, detect if any leaf element is marked as an input (as it will get emitted as flipped). Those operands must be routed through a Wire (this will remove all directionality) before used in the Mux.

  2. In FIRRTL: You can relax equivalence (ignore direction) and drop the passive restriction but generate a result is always passive. Alternatively, you can introduce a 'passivate' expression (unary operator) that takes any value and returns a passive variant (no parts are flipped but all output parts are driven from the corresponding input part). Then, a Chisel3 Mux passivates its operands (or, at least, whenever it detects a flipped field in one). [Quoted text hidden]

    Jim Lawson Wed, Aug 17, 2016 at 4:08 PM

To: Stephen Twigg , Adam Izraelevitz Cc: Jack Koenig , Jonathan Bachrach I think option 1 (address the issue in the PR) is the expedient solution here. In discussing this during the Chisel meeting, it seems we also want to preserve the "original" Chisel3 data structures. Although the data structures:

output io : {flip imem : {flip ready : UInt<1>, valid : UInt<1>, bits :

{btb : {valid : UInt<1>, ...

and:

output io : {imem : {ready : UInt<1>, flip valid : UInt<1>, bits : {btb

: {flip valid : UInt<1>, ...

are equivalent in the Chisel3 sense, they aren't equivalent as far as firrtl is concerned.

This would argue in favor of preserving more of the current Chisel3 code, and incorporating Stephen's PR (and its data structures) as validation tests which can optionally be run prior to emitting firrtl code. [Quoted text hidden]

Adam Izraelevitz Wed, Aug 17, 2016 at 4:10 PM

To: Jim Lawson Cc: Stephen Twigg , Jack Koenig , Jonathan Bachrach Agreed. To summarize the meeting, I think we agreed that Chisel should do all the checking (and expanding of connects), but to still emit the same firrtl types as it does now. I also think the "passivate" expression is a very good idea and worth exploring. [Quoted text hidden]

On Sat, Jan 21, 2017 at 4:18 PM, Richard Lin notifications@github.com wrote:

Another reason would be to expose hierarchical resolved directionality data (i.e. unspecified, input, output, bidirectional, mixed specification), which would be a better API than calling flatten and inspecting the leaf elements (which kind of violates the level of abstraction of a Bundle). This would also allow better node type checks (unbound, directionality-bound, terminally bond) when using nodes as synthesizable types vs. models.

The justification for firrtlDirection was that they need hierarchical directionality data to generate FIRRTL's flip semantics.

ducky64 commented 7 years ago

About flatten being part of the Chisel API: yeah, it's not externally visible, but we do use it in utils (which should try to respect our external API) in Decoupled, as a way to check element directionality.

I think implementation-wise it will be exactly what you propose for localDirection. trueDirection would be something that is only valid at leaves and defined as a lazy val that will compute from parent links.

One of the goals would be supporter stricter checks for directionality / bounded-ness, to make it more intuitive for users. For example, it's not immediately obvious when you try to set directionality on a mixed-directionality Bundle, for example Output(Decoupled(...)). Implementation-wise, it doesn't do anything, since Output essentially means "don't flip". So in non-compatibility mode, I would like to have that error out so the user fixes their code to be more clear.

The rules we proposed at the Chisel meeting two weeks ago were:

Benefits for refactoring the Bindings system: code clean-up, and to make implementing the above less hack-ish. With two data sources for directionality, it's hard to reason about whether the code you write is correct in all circumstances,. If there is one source with very limited (bind-once) mutability, it becomes a lot easier. So kind of a reduce technical debt / correct(er) by construction argument.

Flip semantics: I think it makes sense to propagate designer intent as far down as possible, so persevering hierarchical flips is beneficial in the same way as better Verilog naming is.

sdtwigg commented 7 years ago

I disagree completely with your assessment of Output(DecoupledIO(...)). (I think I am correct on what happens now but even so we should care about what 'should' be happening):

If it does anything, Output(DecoupledIO(...)) should be returning a DecoupledIO that has all nested pins set to be outputs (and thus it is 'struct-like' and serving more as a log of what the valid and ready are rather than a transaction-based interface). Any case where Output(something) (with no Flipped or Input on some higher-level Bundle) returning something with an input direction (as-in the verilog will have a port of input in it) is unnecessarily confusing. Similarly, Output or Input coercing to a flip is strange and a somewhat odd perversion of what the user actually wrote. (I mainly fear a return to the issues conflating Input and flip caused in the original chisel3 implementation.)

I feel like if you want to start segregating Bundles into 'those with direction' and 'those without direction' (aka interfaces and structs) then this is something that should be more clearly formalized (since this also feeds into which connectivity operators make sense, and whether said type is appropriate in a IO, Reg, or Wire).

ducky64 commented 7 years ago

Our approach to what you want to see for Output(DecoupledIO(...)) was Output(DecoupledIO(...).wipeDir).

We definitely should start making Bundles less confusing. Less dynamic-typing-like "it does something but maybe not what you expect" and more static-typing-like "it has to be right to do anything".

aswaterman commented 7 years ago

Output(Decoupled(...)) is perfectly well defined, as Stephen said. I think you're trying enforce your preference under the guise of everyone's expectations.

ducky64 commented 7 years ago

Apparently not well-defined / intuitive enough that the current implementation matches what you think it should do:

class LolTester extends BasicTester {
  class Inner extends Module() {
    val io = IO(Output(Decoupled(Bool())))
    io.bits := true.B
  }
  val inner = Module(new Inner())
  stop()
}

generates this FIRRTL:

circuit LolTester :
  module LolTesterInner :
    input clock : Clock
    input reset : UInt<1>
    output io : {flip ready : UInt<1>, valid : UInt<1>, bits : UInt<1>}

    io is invalid
    io is invalid
    io.bits <= UInt<1>("h01") @[Lol.scala 14:13]

  module LolTester :
    <snip>

Specifically note result of IO(Output(Decoupled(Bool()))) is NOT a Bundle of all Outputs:

    output io : {flip ready : UInt<1>, valid : UInt<1>, bits : UInt<1>}

What we all want Output(...) to do on an already-directioned type is another question and can be subject to continued debate. But what it's doing now seems obviously unintuitive and wrong.

sdtwigg commented 7 years ago

The original incarnation of my pull request does this (it was rather simple) and we continue to have this behavior internally. That the extra firrtlDirection complexity layered on top introduced a bug that perverts the emitted FIRRTL does not change that it is perfectly reasonable to expect Output(any data) to return a bunch of outputs.

This seems to be indicative of an extremely alarming trend of complicating the Chisel API with something and then introducing further complications to get out of it. This need for extra function calls just to get Input or Output to do what you want goes along with the new need for .W on all widths....

ducky64 commented 7 years ago

Resolution from today's meeting: will try to make Output will implicitly clear the internal directionality (if needed). The current behavior of IO(Output(Decoupled(Bool()))) is a bug and shouldn't compile if anything is done with it (so there shouldn't be any compatibility implications).

Additionally, we will standardize some terminology:

azidar commented 7 years ago

Fixed by #617.