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.

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

Fix verbose print of instrs that omit low imm bits #45

Merged
merged 1 commit into from
May 15, 2024

Conversation

elliotb-lowrisc
Copy link
Contributor

Modify unscatter so it works for instructions that do not encode the lowest bits of the immediate value provided to them. This issue has been hidden because many instructions have been encoded using the lower immediate bits rather than upper immediate bits (e.g. imm[19:0] rather than imm[31:12] for lui), and others (which do omit lower immediate bits) are not included in disassembly output (e.g. c_lui with empty rv_c_disass).

See also:
#41

Modify `unscatter` so it works for instructions that do not encode
the lowest bits of the immediate value provided to them.
This issue has been hidden because many instructions have been encoded
using the lower immediate bits rather than upper immediate bits
(e.g. `imm[19:0]` rather than `imm[31:12]` for `lui`),
and others (which do omit lower immediate bits) are not included
in disassembly output (e.g. `c_lui` with empty `rv_c_disass`).

See also:
CTSRD-CHERI#41
@elliotb-lowrisc
Copy link
Contributor Author

I discovered this issue when testing the CHERIoT auicgp instruction, which I defined in RV32_Xcheri.hs as:

auicgp_raw                         =                                        "imm[31:12] cd[4:0] 1111011"
auicgp cd imm                      = encode auicgp_raw                       imm       cd
rv32_xcheri_disass = [ ...
                     , auicgp_raw                      --> prettyU "auicgp"

Copy link
Member

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me, but I will leave it to someone with more Haskell knowledge to approve this.

@gameboo
Copy link
Member

gameboo commented May 14, 2024

I think this makes sense, thanks for the PR. I just want to ping @mn416 for a sanity check on that.

@gameboo gameboo requested a review from mn416 May 14, 2024 15:58
@mn416
Copy link

mn416 commented May 14, 2024

Looks fine to me, other than the apparent inconsistency w.r.t. lui raised by @elliotb-lowrisc. (Should auicgp and lui not be defined in the same style?)

@PeterRugg
Copy link
Collaborator

Happy to merge this: the inconsistency is a broader issue.

@PeterRugg PeterRugg merged commit 73e0933 into CTSRD-CHERI:master May 15, 2024
2 checks passed
@elliotb-lowrisc elliotb-lowrisc deleted the unscatter branch August 2, 2024 11:39
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.

5 participants