[FIRRTL] GrandCentral: Support firrtl.view #8092
Conversation
In addition to existing annotation-based view and companion module handling, extend GrandCentral to support the firrtl.view operation. The goal is to eventually remove the annotation and notion of companion module, in favor of firrtl.view and layers.
| // | ||
| // CHECK: firrtl.module @InterfaceGroundType() | ||
| // CHECK: firrtl.instance companion | ||
| // CHECK-NOT: lowerToBind |
There was a problem hiding this comment.
Modules containing the view are not special.
| // CHECK: sv.interface @GroundView | ||
| // CHECK-SAME: comment = "VCS coverage exclude_file" | ||
|
|
||
| // CHECK-NOT: output_file = #hw.output_file<"gct-dir{{/|\\\\}}" |
There was a problem hiding this comment.
Generated interfaces do not interact with the annotation indicating where grand central bits should go.
The output directory behavior of the generated interfaces should work as part of the ("new") output directory behavior, such that if part of a layer that's extracted it goes with it (usually), so on.
I'm not sure yet how to best slot that in -- functionally, I think what we want is for AssignOutputDirs to propagate through to these instances (and the generated (sub-)Interface's?).
Thoughts?
There was a problem hiding this comment.
Not interacting with the annotation is exactly the correct behavior.
Yes, this should be handled by AssignOutputDirs and, with the glaring exception of the verbatim-instantiated vector interfaces, that may just work (as long as the instance graph understands that sv.interface.instance is an instantiation.
The question is how to handle this for the verbatim instantiations. The ideal approach is to fix sv.interface to be module like. This is a lift. Barring that, adding a discardable attribute on the top-level sv.interface.instance operations that is a list of all the interfaces under them may be a not-too-bad approach.
I think we should also unique the interfaces at this point so there are no shared sub-interfaces between top-level interfaces. As we talked about, the actual interface names don't matter so much for the end user, only the interface instance name is likely meaningful.
All this can be handled in a follow-up.
| if (index >= view.getNumOperands()) { | ||
| view.emitError("more ground types found than view has operands"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Once we iterate (after this PR lands) on the Chisel/FIRRTL intrinsic representation for this, we can move this into a verifier. I'm good with keeping this in the pass for now and, as you see already everywhere, there is tons of verification that only happens during this pass (but is extremely rare to hit as Chisel won't screw up the annotation / intrinsic representation).
| // This is the new style of XMRs using RefTypes. The value substitution | ||
| // index is set to -1, as it will be incremented when generating the | ||
| // string. | ||
| // Generate the path from the LCA to the module that contains the leaf. |
There was a problem hiding this comment.
The last sentence of this comment is, I think, now stale. (I think it's stale in the copied code, too, as there shouldn't be any LCA computation anymore.)
| @@ -642,6 +642,38 @@ struct GrandCentralPass | |||
| SmallVector<VerbatimXMRbuilder> &xmrElems, | |||
| SmallVector<InterfaceElemsBuilder> &interfaceBuilder); | |||
|
|
|||
| // COPIES OF ABOVE FOR NEW FIRRTL.VIEW | |||
There was a problem hiding this comment.
Nit: can the PR change this shouting comment into something more obvious with a start and end? E.g.,:
//===----------------------------------------------------------------------===//
// Intrinsic Handling Functions
//
// Note: these are _intentional_ copies of earlier functions.
//===----------------------------------------------------------------------===//
<awesome copied functions>
//===----------------------------------------------------------------------===//
// End of intrinsic handling functions
//===----------------------------------------------------------------------===//
It might benefit to add a block of text for the annotation-handling functions.
| // Recursively walk the AugmentedBundleType to generate interfaces and XMRs. | ||
| // Error out if this returns None (indicating that the view is | ||
| // malformed in some way). A good error message is generated inside | ||
| // `traverseViewBundle` or the functions it calls. |
There was a problem hiding this comment.
This comment is a bit far from the call to traverseViewBundle.
Looking now, this is in the original code. 🙃 Who wrote that?
| removalError = true; | ||
| continue; | ||
| } | ||
| assert(index == view.getNumOperands()); |
There was a problem hiding this comment.
Should this be an assertion or should it be an error and pass failure? (Is this checked somewhere else?)
| // CHECK: sv.interface @GroundView | ||
| // CHECK-SAME: comment = "VCS coverage exclude_file" | ||
|
|
||
| // CHECK-NOT: output_file = #hw.output_file<"gct-dir{{/|\\\\}}" |
There was a problem hiding this comment.
Not interacting with the annotation is exactly the correct behavior.
Yes, this should be handled by AssignOutputDirs and, with the glaring exception of the verbatim-instantiated vector interfaces, that may just work (as long as the instance graph understands that sv.interface.instance is an instantiation.
The question is how to handle this for the verbatim instantiations. The ideal approach is to fix sv.interface to be module like. This is a lift. Barring that, adding a discardable attribute on the top-level sv.interface.instance operations that is a list of all the interfaces under them may be a not-too-bad approach.
I think we should also unique the interfaces at this point so there are no shared sub-interfaces between top-level interfaces. As we talked about, the actual interface names don't matter so much for the end user, only the interface instance name is likely meaningful.
All this can be handled in a follow-up.
In addition to existing annotation-based view
and companion module handling, extend GrandCentral to support the firrtl.view operation.
The goal is to eventually remove the annotation
and notion of companion module in favor of layers.
The code here is mostly copies of what exists already, with minor alterations for
firrtl.viewinstead of the annotations.The plan is to support both until fully using the intrinsic and drop the old support by deleting the old copies.