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.

Sign up for GitHub

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

Jump to bottom

Fix Decodetable #3563

Merged
merged 3 commits into from
Oct 16, 2023
Merged

Fix Decodetable #3563

merged 3 commits into from
Oct 16, 2023

Conversation

sequencer
Copy link
Member

  • bug fix for decoderbundle not being able to be lhs op
  • add default field for DecodeField for DecodeTable non X case

@sequencer sequencer added the Bugfix Fixes a bug, will be included in release notes label Oct 2, 2023
@sequencer sequencer added this to the 3.6.x milestone Oct 2, 2023
@sequencer sequencer marked this pull request as ready for review October 2, 2023 15:50
@sequencer sequencer requested a review from jackkoenig October 6, 2023 04:11
@sequencer
Copy link
Member Author

Ask for review, cc @jackkoenig

jackkoenig
jackkoenig reviewed Oct 13, 2023
View reviewed changes
src/main/scala/chisel3/util/experimental/decode/DecoderBundle.scala

val table: TruthTable = TruthTable(
patterns.map { op => op.bitPat -> fields.reverse.map(field => field.genTable(op)).reduce(_ ## _) },
fields.reverse.map(_.default).reduce(_ ## _)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this part of the change, this feels very different than the previous use of BitPat.dontCare.

Copy link
Member Author

Choose a reason for hiding this comment

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

because some time we don't use don't care as our default selection(e.g. illinstr will default be false), thus I added a default field to let user choose their own default.

@sequencer sequencer mentioned this pull request Oct 14, 2023
Draft
@sequencer sequencer force-pushed the decodetable_fix branch 2 times, most recently from ebff827 to 5ac7402 Compare October 15, 2023 17:54
@sequencer sequencer requested a review from jackkoenig October 15, 2023 17:54
jackkoenig
jackkoenig approved these changes Oct 16, 2023
View reviewed changes
Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

Approved but please add a test.

@sequencer sequencer added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Oct 16, 2023
@mergify mergify bot merged commit 7466789 into main Oct 16, 2023
@mergify mergify bot deleted the decodetable_fix branch October 16, 2023 19:45
@mergify mergify bot added the Backported This PR has been backported label Oct 16, 2023
mergify bot pushed a commit that referenced this pull request Oct 16, 2023
@sequencer @mergify
Fix Decodetable (#3563)
804676e 
* bug fix for decoderbundle not being able to be lhs op

* add default field for DecodeField for DecodeTable non X case

* add test to show DecodeField default use case

(cherry picked from commit 7466789)
mergify bot pushed a commit that referenced this pull request Oct 16, 2023
@sequencer @mergify
Fix Decodetable (#3563)
7daf3c4 
* bug fix for decoderbundle not being able to be lhs op

* add default field for DecodeField for DecodeTable non X case

* add test to show DecodeField default use case

(cherry picked from commit 7466789)
This was referenced Oct 16, 2023
Merged
Merged
mergify bot added a commit that referenced this pull request Oct 16, 2023
@mergify @sequencer
Fix Decodetable (#3563) (#3580)
e845191 
* bug fix for decoderbundle not being able to be lhs op

* add default field for DecodeField for DecodeTable non X case

* add test to show DecodeField default use case

(cherry picked from commit 7466789)

Co-authored-by: Jiuyang Liu <liu@jiuyang.me>
mergify bot added a commit that referenced this pull request Oct 17, 2023
@mergify @sequencer
Fix Decodetable (#3563) (#3579)
818cb70 
* bug fix for decoderbundle not being able to be lhs op

* add default field for DecodeField for DecodeTable non X case

* add test to show DecodeField default use case

(cherry picked from commit 7466789)

Co-authored-by: Jiuyang Liu <liu@jiuyang.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackkoenig jackkoenig jackkoenig approved these changes

Assignees
No one assigned
Labels
Backported This PR has been backported Bugfix Fixes a bug, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Milestone
3.6.x
Development

Successfully merging this pull request may close these issues.
2 participants
@sequencer @jackkoenig