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

When to use <- or <= #52

Closed
keszocze opened this issue Dec 23, 2022 · 12 comments
Closed

When to use <- or <= #52

keszocze opened this issue Dec 23, 2022 · 12 comments

Comments

@keszocze
Copy link
Contributor

I am wondering when exactly the single line arrow <- is used to connect things. In the Partial Connects Section they are used

  • in the first example, performing an actual partial connection:
module MyModule :
   input myinput: {flip a: UInt, b: UInt[2]}
   output myoutput: {flip a: UInt, b: UInt[3], c: UInt}
   myoutput <- myinput
  • in the second, equivalent, example to fully connect parts of the bundle/vector entries:
   myinput.a <- myoutput.a
   myoutput.b[0] <- myinput.b[0]
   myoutput.b[1] <- myinput.b[1]

I searched the document for further occurrences of <- and found none. What exactly are the semantics of <-? It does not seem to be a typo as <- is also used to define the grammar in the Language Definition Section

I propose to either

  1. remove <- from the spec or
  2. extend the part on partial connections, formally introducing and explaining <-.

I am very much in favor of 1 as it simplifies the grammar and I do not see the need for a distinction between partial and non-partial assignments in the first place.

If there is a consensus here, I will happily prepare a corresponding pull request.

@mwachs5
Copy link
Collaborator

mwachs5 commented Dec 23, 2022

I think you could be right that the eventual (near future?) move is to remove it from the spec. Current chisel3 project emits <- to match the permissive legacy connection semantics of its language, but chisel3 recently introduced new connection operators that don't emit <- and deprecated the code that would emit them.

But neither of those changes are yet in an official supported version of chisel3 which is a major producer of .fir IR, so removing the operator is not something we could commit immediately to the spec (as it would be a major change and we may want to coordinate ordering of other major breaking changes). But feel free to open a PR!

@mwachs5
Copy link
Collaborator

mwachs5 commented Dec 23, 2022

The main difference between the two is that there can be missing fields or mismatched UInt width fields in a <-:

Two oriented bundle types are not equivalent if there exists two fields, one from each oriented bundle type, that have identical names but whose oriented types are not equivalent. Otherwise, the oriented bundle types are equivalent.

The <= doesn't let you have missing fields. And all widths have to match.

I'm not sure why you say there is no formal definition of it, it is described at the same level as the Connect is in the "Partial Connect" section: https://github.com/chipsalliance/firrtl-spec/blob/main/spec.md#partial-connects just after the Connect sectinon. Which i guess you found since the example you pasted is from there, which shows that the c missing field is allowed in the <-, and the b with a different width is allowed to connect.

@keszocze
Copy link
Contributor Author

My confusing might stem from the fact that the second example also uses <-. This seems to suggest that the operators might be used interchangeably (at least I guess that's why I asked for clarification in the first place).

So a quick idea that does not break anything would be

  • to change the second example to
   myinput.a <= myoutput.a
   myoutput.b[0] <= myinput.b[0]
   myoutput.b[1] <= myinput.b[1]

stressing the difference in the connection and

  • to add a sentence that is as clear as your

The <= doesn't let you have missing fields. And all widths have to match.

above. I can prepare a PR for that if the idea sounds good.

@mwachs5
Copy link
Collaborator

mwachs5 commented Dec 23, 2022

I'm actually now wondering if

   myoutput.b[0] <= myinput.b[0]
   myoutput.b[1] <= myinput.b[1]

(or the way it was written before) is even legal firrtl, i thought we didn't support that on the LHS until #26 lands

a better way would be showing the sub-extraction on the RHS https://github.com/chipsalliance/firrtl-spec/blob/main/spec.md#bit-extraction-operation

myoutput.b <= bits(myinput.b, 1, 0)

with maybe a comment on what it's actually doing since we haven't really explained bits yet

@seldridge
Copy link
Member

seldridge commented Dec 23, 2022

Yeah, the example should really just be:

myinput.a <= my output.a
myoutput.b <= myoutput.b

IMO, it would be better to show this as a vector and a bundle as this is where the behavior of connect and partial connect differ. I.e., the original point of partial connect <- is that it connects only matching fields (for bundles) or only matching elements (for vectors). Both connect (<=) and partial connect (<-) have bit width truncation behavior. This means that they are entirely equivalent for ground types. I.e., using illegal syntax of myoutput.b[0] to show truncation isn't really useful.

I'd suggest the following two examples.

This shows the behavior of partial connect for vectors:

circuit Foo :
  module Foo :
    input a: UInt<1>[2]
    output b: UInt<1>[3]
    output c: UInt<1>[1]

    b is invalid
    b <- a
    c <- a

This becomes (firtool Tmp.fir -parse-only | circt-translate -export-firrtl | sed 's/@.*//'):

circuit Foo :
  module Foo :
    input a : UInt<1>[2]
    output b : UInt<1>[3]
    output c : UInt<1>[1]

    b is invalid 
    b[0] <= a[0] 
    b[1] <= a[1] 
    c[0] <= a[0] 

The bundle equivalent is:

circuit Foo :
  module Foo :
    input a: {a: UInt<1>, b: UInt<1>}
    output b: {a: UInt<1>, b: UInt<1>, c: UInt<1>}
    output c: {a: UInt<1>}

    b is invalid
    b <- a
    c <- a

This becomes:

circuit Foo :
  module Foo :
    input a : { a : UInt<1>, b : UInt<1> }
    output b : { a : UInt<1>, b : UInt<1>, c : UInt<1> }
    output c : { a : UInt<1> }

    b is invalid 
    b.a <= a.a 
    b.b <= a.b 
    c.a <= a.a 

Both of these examples are illegal if the partial connects are replaced with connects.

Going forward, I think we will likely drop partial connect <- and move to two narrow flavors of connects:

  1. connect (<=) which has the same semantics as the existing connect operator
  2. strict connect which requires bit widths must match

This connect/strict connect split is what we use in the MLIR-based FIRRTL Compiler (MFC, i.e., CIRCT). All partial connects are removed during parsing and everything is converted to strict connect (with appropriate pads/tails) when possible (and connect will canonicalize to strict connect for convenience). Unknown widths prevents everything from being converted to strict connect immediately, though.

This results in a relatively verbose IR, however, this can be remedied with additional operators bundlecreate and vectorcreate (both of which we are using inside the MFC's FIRRTL Dialect.

@mwachs5
Copy link
Collaborator

mwachs5 commented Dec 24, 2022

Now I am confused, I read what you have written above @seldridge as it is legal to do

input a: UInt<3>
output b: UInt<2>

b <= a

but the Connect section of the spec has this condition 2:

In order for a connection to be legal the following conditions must hold:

1. The types of the left-hand and right-hand side expressions must be equivalent (see [@sec:type-equivalence] for details).
2. The bit widths of the two expressions must allow for data to always flow from a smaller bit width to an equal size or larger bit width.
...

Condition 1 is OK because the bit widths don't matter for Uint type equivalence, as clearly stated. But then what is the (additional, only for Connect not Partial Connect) condition 2 trying to stay, if not that the above is illegal?

@seldridge
Copy link
Member

Great point @mwachs5. The spec is actually wrong here!

The SFC never obeyed this and did implicit truncation for connect as well. The MFC then inherited this behavior.

For practical reasons, we should change the spec to align with the implementations.

Example:

circuit Foo :
  module Foo :
    input a: UInt<2>
    output b: UInt<3>
    output c: UInt<1>

    b <= a
    c <= a

SFC (firrtl -I Foo.fir && cat Foo.v):

module Foo(
  input  [1:0] a,
  output [2:0] b,
  output       c
);
  assign b = {{1'd0}, a};
  assign c = a[0];
endmodule

MFC (firtool Foo.fir):

// Generated by CIRCT unknown git version
module Foo(
  input  [1:0] a,
  output [2:0] b,
  output       c);

  assign b = {1'h0, a};
  assign c = a[0];
endmodule

For completeness of the example, if this is changed to partial connect it behaves the same:

circuit Foo :
  module Foo :
    input a: UInt<2>
    output b: UInt<3>
    output c: UInt<1>

    b <- a
    c <- a

SFC:

module Foo(
  input  [1:0] a,
  output [2:0] b,
  output       c
);
  assign b = {{1'd0}, a};
  assign c = a[0];
endmodule

MFC:

// Generated by CIRCT unknown git version
module Foo(
  input  [1:0] a,
  output [2:0] b,
  output       c);

  assign b = {1'h0, a};
  assign c = a[0];
endmodule

@keszocze
Copy link
Contributor Author

Thank you very much for the discussion. This clarified a lot for me.

Is there a decision on changing the spec and if so, how? I am more than happy with creating a PR if that is helpful.

@mwachs5
Copy link
Collaborator

mwachs5 commented Jan 3, 2023

Is there a decision on changing the spec and if so, how? I am more than happy with creating a PR if that is helpful.

@keszocze yes, i think we should:

  • remove the statement about "2. The bit widths of the two expressions must allow for data to always flow from a smaller bit width to an equal size or larger bit width."
  • Fix the examples as described by @seldridge in When to use <- or <=  #52 (comment)

I think we can have this as a patch since while it is technically changing the spec, it's just aligning the spec with the only two known implementations of it, so we could consider it a bugfix

@azidar
Copy link
Collaborator

azidar commented Jan 3, 2023

Mea culpa!

Yup. Thank you @keszocze for pointing this out. Indeed. SFC (and now MFC) always implicitly truncate the connections. I believe originally that this width restriction on connections was only for FIRRTL in Low Form, but something was lost in translation and the spec wasn't updated.

@seldridge
Copy link
Member

Connect is now marked as truncating after 9d040da.

@seldridge
Copy link
Member

Partial connect is now gone after d569a5a.

This should fully close this issue. Feel free to re-open if there's more to discuss!

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

4 participants