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

Initial support for E extension #646

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

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 factors out the I base register system, with its 32 registers, and then offers a parallel instantiation for the E base register system, with its 16 registers.

Note that this is subtly wrong for RVnnEZicsr systems until #645 or something much like it lands.

@nwf nwf changed the title 202412 newtype regidx eext Initial support for E extension Dec 17, 2024
Copy link

Test Results

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

Results for commit 241f9b0. ± Comparison against base commit 5f4a8e6.

@Timmmm
Copy link
Collaborator

Timmmm commented Dec 18, 2024

It would be really nice to have this as an "initialisation time" option rather than yet another compile time option. I think @Alasdair 's new config system will take care of regidx. For the other stuff in riscv_regs_e.sail can you keep a single riscv_regs.sail and use runtime checks and add an extensionEnabled(Ext_E)? E.g.


function rX (regno(r) : regno) -> xlenbits = {
  let v : regtype =
    match r {
      0 => zero_reg,
      1 => x1,
      2 => x2,
      3 => x3,
      4 => x4,
      5 => x5,
      6 => x6,
      7 => x7,
      8 => x8,
      9 => x9,
      10 => x10,
      11 => x11,
      12 => x12,
      13 => x13,
      14 => x14,
      15 => x15,
      16 if not(extensionEnabled(Ext_E)) => x16,
      17 if not(extensionEnabled(Ext_E)) => x17,
...
      _  => {assert(false, "invalid register number"); zero_reg}
    };
  regval_from_reg(v)
}

I think there's no harm having x16 etc still existing, even if E is enabled.

@Timmmm
Copy link
Collaborator

Timmmm commented Dec 18, 2024

That also makes it easier to support runtime enabling/disabling of E, by writing misa.I.

@Timmmm
Copy link
Collaborator

Timmmm commented Dec 18, 2024

Although... if we do support that then you can't change the regidx type. Hmm.

@Alasdair
Copy link
Collaborator

We could have a split between a compile-time only E build where regidx is 4 bits and a runtime E mode where it just restricts the set of available GPRs. The specification does specifically mention that E can be enabled and disabled by writing misa[I] so we do need to support the second option.

@nwf
Copy link
Contributor Author

nwf commented Dec 18, 2024

Certainly the compile-time E-only option is a more attractive base for CHERIoT, and I could imagine utility for a compile-time I-only option (as in this PR), but maybe that's less useful. I think Someone Who Isn't Me gets to do the dynamic E/I case, but I would love to see it done and hopefully this PR is at least not a terrible place to start.

@Timmmm
Copy link
Collaborator

Timmmm commented Dec 18, 2024

Is there any point having the compile-time version if we support the runtime version though and make misa[I] read only?

I guess it helps with the formal and SV backends?

I think SWIM gets to do the dynamic E/I case

What's SWIM?

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