fix: corner cases in aggregate generation #9036
Open
+56
−11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit fixes two different corner cases in aggregate generation
when running with firrtl generated by chipyard
1. a clock signal could reside in an "aggregate". This is caused by
diplomacy in part: %6 = "hw.aggregate_constant"() <{fields =
[[[false, false]]]}> : () -> !hw.struct<member:
!hw.struct<fake_uncore: !hw.struct<clock: !seq.clock, reset: i1>>>
2. Some FIRRTL aggregates still arrive as a DictionaryAttr keyed by
field name. Trying to cast that dictionary to an array could create a
segfault. The helper now handles both array and dictionary forms: for
dictionaries the HW struct fields by name and build the ordered array
we need, so the rest of the lowering stays safe.
Signed-off-by: Tianrui Wei <tianrui@tianruiwei.com>
|
This fixes two different bugs encountered when using the
|
|
Hi thank you for sending the PR! I think the root cause of (1) is the type implementation of HW. Currently HW struct type doesn't allow seq.clock as an element so it is causing a failure. For (2) I'm not sure it actually happens. Do you have a test case for that? |
|
@uenoku Thanks for the quick response. For <1>, do you think my changes make sense? I'm not sure if the HW struct type disallows seq.clock is a part of firrtl spec (obviously you'd be the expert :) ), but if so, I'll redirect the issue to diplomacy, which is doing chisel hacks to pack clocks inside structs For <2>, yes, the fir file very huge though (and are causing issues in circt-reduce), would you like me to upload it? It could also be obtained by running chipyar with the |
Yes, I think the change for aggregate constant makes sense.
I think firrtl spec is not restricting anything about output verilog types (except for public module ports which must be scalarized). So firtool has a freedom to emit structs or scalarized ports. The issue is mostly implementation of HW type system which makes it hard to use seq.clock. #8931 is similar issue and we cannot use It's necessary to modify HW type implementation to allow seq.clock in HW type, but I'm also not sure what's the best way to do that. From that reason previously we thought it's easier to just lower aggregate that contain clock ( |
|
@uenoku Thanks for your feedback. I agree with your approach, and I think the current PR might be a little brute-forcy in terms of forcing the clocking conversion, it would be a lot cleaner if this is fixed upstream in the hw type implementation. Please lmk your thoughts on rebasing #4667, and I'll be happy to re-implement this PR with the landed changes. |
|
#4667 is basically giving up preserving clock in bundle/array. Is that ok for you? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes two different corner cases in aggregate generation when running with firrtl generated by chipyard
a clock signal could reside in an "aggregate". This is caused by diplomacy in part: %6 = "hw.aggregate_constant"() <{fields = [[[false, false]]]}> : () -> !hw.struct<member:
!hw.struct<fake_uncore: !hw.struct<clock: !seq.clock, reset: i1>>>
Some FIRRTL aggregates still arrive as a DictionaryAttr keyed by field name. Trying to cast that dictionary to an array could create a segfault. The helper now handles both array and dictionary forms: for dictionaries the HW struct fields by name and build the ordered array we need, so the rest of the lowering stays safe.