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

Simplify Zicsr AST #645

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

nwf
Copy link
Contributor

@nwf nwf commented Dec 17, 2024

Split out from and sitting atop #617, the last commit here is the salient one. This transforms a bool * regidx under one ast data constructor into two ast data constructors, allowing us to stop punning between regidx and bits(5). This will be significant in light of #617 for supporting the E extension, where regidx will be (isomorphic to) bits(4); see #646.

Copy link

github-actions bot commented Dec 17, 2024

Test Results

396 tests  ±0   396 ✅ ±0   0s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 35eb511. ± Comparison against base commit 59c852c.

♻️ This comment has been updated with latest results.

@Timmmm
Copy link
Collaborator

Timmmm commented Dec 18, 2024

Ah I understand the motivation now. In order to avoid losing the is_imm information could we use a union instead?

union RegOrImm = {
  Reg : regidx,
  Imm : bits(5),
}

@nwf
Copy link
Contributor Author

nwf commented Dec 18, 2024

I am... reasonably certain we haven't lost any information and that I haven't changed semantics. Rather than pass in a union and conditionally call X(), I just moved the discrimination into ast itself and had the appropriate arm unconditionally call X(). Can you say why you think a union is beneficial?

Copy link
Collaborator

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

I like the split into two separate AST clauses here but I think this changes the behaviour to no longer be correct.

nwf added 4 commits January 3, 2025 23:49
This required splitting many of the AST constructors.
Zicsr instructions can take an immediate or a register index for their
source operand.  Don't pun in the AST using a boolean to distinguish,
and instead use two constructors in the AST scattered union.
@nwf nwf force-pushed the 202412-newtype_regidx-zicsr branch from e46e077 to 35eb511 Compare January 3, 2025 23:53
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.

3 participants