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

Update Initialization Coverage spec for zero-width #156

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

albertchen-sifive
Copy link

Specify that non-zero-width components must be initialized, while zero-width components are implicitly initialized with value zero.

Specify that non-zero-width components must be initialized, while
zero-width components are implicitly initialized with value zero.
Copy link
Collaborator

@darthscsi darthscsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this rule interact with uninferred widths? Are those assumed to be non-zero width until proven otherwise?

@@ -1616,8 +1616,10 @@ module MyModule :
```

This is an illegal FIRRTL circuit and an error will be thrown during compilation.
All wires, memory ports, instance ports, and module ports that can be connected to must be connected to under all conditions.
All non-zero-width wires, memory ports, instance ports, and module ports that can be connected to must be connected to under all conditions.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the next are ambiguous as it's not clear (at least looking at each sentence in isolation) whether "non-zero-width" only applies to wires or to the entire list.

Copy link
Collaborator

@mwachs5 mwachs5 Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree that there seems to be a difference in use case for 0-width elements (SInt, UInt, Analog) and 0-element Aggregates (Vec, bundle types). To me it seems like users should be responsible for initializing the first one, while the latter does make sense to allow to not happen in the spec. I'm actually not sure what the proposed spec change is currently trying to cover --both of these cases?

Registers do not need to be connected to under all conditions, as it will keep its previous value if unconnected.
Zero-width wires, memory ports, instance ports, and module ports do not need to be connected.
These are implicitly initialized with a value of zero.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is initialized the right way to frame this? Don't we elsewhere say that reads of zero-width are zero? If that is true why bring initialization into anything?

@mmaloney-sf mmaloney-sf self-requested a review December 7, 2023 22:09
@@ -1616,8 +1616,10 @@ module MyModule :
```

This is an illegal FIRRTL circuit and an error will be thrown during compilation.
All wires, memory ports, instance ports, and module ports that can be connected to must be connected to under all conditions.
All non-zero-width wires, memory ports, instance ports, and module ports that can be connected to must be connected to under all conditions.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be mindful of the ambiguity in this sentence.

Do I "parse" this sentence as:

  • "All (non-zero-width wires), memory ports, instance ports, and module ports ...", or as
  • "All non-zero-width (wires, memory ports, instance ports, and module ports) ..."

?

Also, zero-width needs a definition (even if that definition isn't "obvious"). This is part of the ongoing initiative I have for the spec, where we need to be more explicit about semantics.

Copy link
Collaborator

@darthscsi darthscsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bad idea which complicates the IR. Whether chisel could choose to invalidate zero-width values behind the scenes is a different question.

IR should be consistent and not littered with special cases. allowing exactly two of the integer types to be exceptions to initialization rules is a special case.

I would argue this also encourages people to write more fragile code, but again, chisel could choose to allow that independent of this.

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

Successfully merging this pull request may close these issues.

4 participants