Skip to content

Conversation

sequencer
Copy link
Member

@sequencer sequencer commented Sep 26, 2023

Original IDecode.scala is really hard to maintain which use the view from each instruction rather than each control field.
This PR remove original decoder with new [DecodeTable] in Chisel.
It will be a non-small refactor, but it can help us adding new instructions and have a better understand to the uarch.
After this PR is landed, all original IDecode will be moved into an "legacy" folder, downstream user can still use them if they wish, but after Boom and Xiangshan migrated, they will be removed. cc @jerryz123 , @poemonsense
sequncer/rvdecoderdb will be upstreamed to chipsalliance as a part of RC after all test clean.

BTW, I found the Zb/Zk UOP was really a bad design...

Type of change: other enhancement

Impact: API addition (no impact on existing code)

Development Phase: proposal | implementation

Release Notes
Switch to DecodeTable API.

@poemonsense
Copy link
Contributor

We are still on the way to upgrading to Chisel 5.

Thank you for cc me. We will bump rocket first to the version before new decoder table and then bump to the latest with changes on the decoder table.

Btw, we still have some local commits in Openxiangshan/rocket-chip. I'll see whether they should/could be upstreamed to chipsalliance. I agree it would be better to follow upstream rocket.

@sequencer sequencer force-pushed the new_decoder branch 2 times, most recently from e5d136b to 71a0824 Compare October 1, 2023 23:06
@sequencer
Copy link
Member Author

Spot my bug in chipsalliance/chisel#2924
https://github.com/chipsalliance/chisel/blob/440f01addeadd265fca2518c0a4df00b698e4603/src/main/scala/chisel3/util/experimental/decode/DecoderBundle.scala#L61
asTypeOf should never be LHS, otherwise it will create a wire and broken in when connection....
I'll vendor a chisel source for debugging, but not going to be merged.

@sequencer sequencer force-pushed the new_decoder branch 8 times, most recently from ee9d52d to 1ff5e30 Compare October 14, 2023 03:28
@sequencer
Copy link
Member Author

sequencer commented Oct 14, 2023

Assign the Zb/Zk to @cyyself, I'll skip them in this PR by mask them off.

After Chisel merging chipsalliance/chisel#3563 I'll bump Chisel version to 3.6.1 and 5.0.x and get this PR merged, CI for this PR is based on my hotfix branch in Chisel.

@sequencer sequencer force-pushed the new_decoder branch 5 times, most recently from 4599f46 to 168524d Compare October 17, 2023 11:01
@cyyself cyyself mentioned this pull request Nov 18, 2023
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.

2 participants