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.

nwf added 4 commits December 13, 2024 20:33
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.
Copy link

Test Results

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

Results for commit e46e077. ± Comparison against base commit 5f4a8e6.

@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?

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