Implement FIRRTL type alias mechanism for Bundles

[jared-barocsi]

Adds a simple mechanism to the Bundle type that emits a FIRRTL type alias statement as detailed in the FIRRTL spec.

This is achieved by overriding an aliasName: Option[String] with a non-empty string, resulting in a type ... = { ... } statement emitted in the FIRRTL. Additionally, any further definitions that normally use the bundle type now use the alias as the type.

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you add appropriate documentation in docs/src?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

• Feature (or new API)

Desired Merge Strategy

Squash

Release Notes

Add aliasName to Bundles: a way for users to define a type alias for a bundle type, resulting in the emission and usage of alias type statements in FIRRTL.

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
• Did you mark the proper milestone (Bug fix: 3.5.x, 3.6.x, or 5.x depending on impact, API modification or big change: 6.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you do one of the following when ready to merge:
  • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
  • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

[dtzSiFive]

Suggested change

	case DefTypeAlias(info, name, tpe) => b ++= "type "; b ++= name; b ++= " = "; s(tpe); s(info)
	case DefTypeAlias(info, name, tpe) => b ++= "type "; b ++= name; b ++= " = "; s(tpe);

Info locators are not supported on type alias statements. They're not in the spec grammar and firtool rejects them, so probably best to omit.

(not sure whether this removal should propagate up to remove it from DefTypeAlias and etc., but maybe?)

[dtzSiFive]

https://github.com/llvm/circt/blob/main/lib/Dialect/FIRRTL/Import/FIRTokenKinds.def Some tokens like stop and force are lexed only if have opening paren too (LPLEYWORD), which I think is the difference here. Hope this helps!

…

On Fri, Sep 1, 2023, 5:40 PM Jack Koenig ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/src/main/scala/chisel3/Data.scala <#3445 (comment)> : > + // Passive direction for this node, ignoring coercion from Input and Output. + private var _passiveDirection: SpecifiedDirection = SpecifiedDirection.Unspecified + private[chisel3] def passiveDirection: SpecifiedDirection = _passiveDirection + I don't think we need this var on all Data because Elements always have passive direction. Considering a var is an extra 4-bytes of memory use, this needs to only be added to Record (Vecs can just check their sample_element). You can have the def on Data and then implement it as true on Element and do the implementation with a var on Record. ------------------------------ In core/src/main/scala/chisel3/Aggregate.scala <#3445 (comment)> : > @@ -1463,4 +1472,59 @@ abstract class Bundle extends Record { * the fields in the order they were defined */ override def toPrintable: Printable = toPrintableHelper(_elements.toList.reverse) + + private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection): Unit = { Why is this implemented on Bundle instead of Record? ------------------------------ In core/src/main/scala/chisel3/internal/package.scala <#3445 (comment)> : > + /** The list of reserved keywords in FIRRTL which result in incorrect parsing when used in any statement outside + * of their correct use cases. + * This is vital for type aliases, which can in principle be named after these keywords but results + * in problematic and incorrect FIRRTL. + * Some words, like 'stop' and 'force', do not result in parser errors and consequently do not show up here. + */ If we're excluding some FIRRTL keywords then I don't think it is correct to call this firrtlKeywords. If this really is just for legalizing type alias names, maybe just giving it name that describes the use case is better, eg. illegalTypeAliasNames or something. ------------------------------ In src/test/scala/chiselTests/TypeAliasSpec.scala <#3445 (comment)> : > + + val io = IO(new Bundle { + val in = Input(new FooBundle) + val out = Output(new FooBundle) + }) + + io.out.x :#= io.in.x + } + + val args = Array("--throw-on-first-error", "--full-stacktrace") + val chirrtl = ChiselStage.emitCHIRRTL(new Test(tpe), args) + }).getMessage should include( + s"Attempted to override a FIRRTL keyword '$tpe' with a bundle type alias. Chisel does not automatically disambiguate aliases using these keywords at this time." + ) + } + } Can you add a test where you mix HasTypeAlias into a Record? From my reading of the code it's legal but not correctly implemented [yet]. ------------------------------ In core/src/main/scala/chisel3/Aggregate.scala <#3445 (comment)> : > + val isFlipped = DataMirror + .collectMembers(this) { case d: Data if d.passiveDirection == SpecifiedDirection.Flip => d } + .toSeq + .nonEmpty + val isCoerced = direction match { + case ActualDirection.Input | ActualDirection.Output => true + case other => false This logic to determine coercedness/strippedness should be integrated into the existing Record.bind code so that we don't have to iterate over the elements an additional time. Furthermore, this needs to be done in a hierarchical way--DataMirror.collectMembers is recursive which means this code is effectively O(n^d) (where n is number of Bundle elements and d is the depth of your Bundle hierarchy), every single Bundle in a nested hierarchy is recursively accessing all fields so every field gets accessed many many times. The purpose of adding a new field like _passiveDirection is so that the value is cached for each Record as you go up the tree and thus you don't need to access the children of any Records. Also, for Vecs you really just need to check the sample_element. I'm not entirely convinced that _passiveDirection is the right information to be storing here so please think it through and figure out what information is needed so that this can be done live inside of Record.bind and not require touching children recursively. ------------------------------ In core/src/main/scala/chisel3/Aggregate.scala <#3445 (comment)> : > + // Filter out (TODO: disambiguate) FIRRTL keywords that cause parser errors if used + if (firrtlKeywords.contains(candidateAlias)) { + Builder.error( + s"Attempted to override a FIRRTL keyword '$candidateAlias' with a bundle type alias. Chisel does not automatically disambiguate aliases using these keywords at this time." + )(sourceInfo) + } else { + // Compute the structural type of this bundle with no subfield aliasing + val thisType = Converter.extractType(this, sourceInfo) + + // If the name is already taken, check if there exists a *structurally equivalent* bundle with the same name, and + // simply error (TODO: disambiguate that name) + if ( + Builder.globalBundleNamespace.contains(candidateAlias) && + Builder.bundleStructuralHashMap.get(candidateAlias).exists(_._2 != thisType) + ) { + val bundleValue = Builder.bundleStructuralHashMap.get(candidateAlias).get + // Conflict found: + Builder.error( + s"Attempted to redeclare an existing bundle type alias '$candidateAlias' with a new bundle structure:\n'$thisType'.\n\nThe alias was previously defined as:\n'${bundleValue._2}${bundleValue._3 + .makeMessage(" " + _)}" + )(sourceInfo) + } else { + if (!Builder.globalBundleNamespace.contains(candidateAlias)) { + Builder.globalBundleNamespace.name(candidateAlias) + Builder.bundleStructuralHashMap.put(candidateAlias, (this, thisType, sourceInfo)) + } + This logic (or at least most of it) should probably live inside of a function in the Builder, it feels like a lot to have inlined here. I'm also worried that this implementation is slow. I'd like to see a benchmark of some large Records and some deeply nested Records using this API to see how it performs. We may need to figure out how to make this structural hashmap hierarchical and efficient. ------------------------------ In src/test/scala/chiselTests/TypeAliasSpec.scala <#3445 (comment)> : > + + val io = IO(new Bundle { + val in = Input(new BarBundle) + val out = Output(new BarBundle) + }) + + io.out :#= io.in + } + + val chirrtl = ChiselStage.emitCHIRRTL(new StandardStrippedTest) + // No unmodified alias for the bidirectional bundle since it isn't actually bound + chirrtl shouldNot include("type Buzz = { x : UInt<8>, flip y : UInt<8>}") + // No unmodified alias for the stripped bundle + chirrtl shouldNot include("type Buzz = { x : UInt<8>, y : UInt<8>}") + // Modified alias for the stripped bundle + chirrtl should include("type Buzz_stripped = { x : UInt<8>, y : UInt<8>}") It is under the user's control now ------------------------------ In core/src/main/scala/chisel3/Data.scala <#3445 (comment)> : > + // Passive direction for this node, ignoring coercion from Input and Output. + private var _passiveDirection: SpecifiedDirection = SpecifiedDirection.Unspecified + private[chisel3] def passiveDirection: SpecifiedDirection = _passiveDirection + With this suggestion you'll want to set _passiveDirection during Record.bind. ------------------------------ In core/src/main/scala/chisel3/internal/package.scala <#3445 (comment)> : > + /** The list of reserved keywords in FIRRTL which result in incorrect parsing when used in any statement outside + * of their correct use cases. + * This is vital for type aliases, which can in principle be named after these keywords but results + * in problematic and incorrect FIRRTL. + * Some words, like 'stop' and 'force', do not result in parser errors and consequently do not show up here. + */ @dtzSiFive <https://github.com/dtzSiFive> @seldridge <https://github.com/seldridge> do either of you know why some keywords would be problematic but others (stop and force) are not? @jared-barocsi <https://github.com/jared-barocsi> It would be best to ensure we know why various keywords are legal alias identifiers and others are not. — Reply to this email directly, view it on GitHub <#3445 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYZWN5PLRAN2BAMWVGB74SLXYJP45ANCNFSM6AAAAAA24CJPOM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mwachs5]

LGTM! Thanks for the docs. though they are more starting to get more to explanation than cookbook, it is better than not having any docs to point to at all.