Skip to content

Conversation

@nadime15
Copy link
Collaborator

@nadime15 nadime15 commented Jun 24, 2025

Add remaining vector crypto extensions and automatically enable superset extensions when all of the appropriate subset extensions are enabled.

@github-actions
Copy link

github-actions bot commented Jun 24, 2025

Test Results

400 tests  ±0   400 ✅ ±0   1m 28s ⏱️ -1s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 96ba8c3. ± Comparison against base commit d8c3006.

♻️ This comment has been updated with latest results.

@nadime15
Copy link
Collaborator Author

nadime15 commented Jun 25, 2025

Does somebody know how things actually work with the device tree and shorthands like these:

Vector Crypto Extensions

Extension Description
Zvkn Zvkned + Zvknhb + Zvkb + Zvkt
Zvknc Zvkn + Zvbc
Zvkng Zvkn + Zvkg
Zvks Zvksed + Zvksh + Zvkb + Zvkt
Zvksc Zvks + Zvbc
Zvksg Zvks + Zvkg

So let’s say I enable Zvknc, which pulls in Zvkn + Zvbc, and those pull in Zvkned + Zvknhb + Zvkb + Zvkt. Should the final device tree only print Zvknc or Zvbc_Zvkn_Zvknc or even Zvbc_Zvkb_Zvkn_Zvknc_Zvkned_Zvknhb_Zvkt.

As far as I can see now its actually printing Zvbc_Zvkb_Zvkn_Zvknc_Zvkned_Zvknhb_Zvkt but the desired behaviour (?) is probably only to print Zvknc.

@pmundkur

@Timmmm
Copy link
Collaborator

Timmmm commented Jun 25, 2025

Yeah I think we can print the minimum necessary if the others are implied. You don't have to always list all the sub-extensions.

@jordancarlin
Copy link
Collaborator

I think the simplest approach is to not include these superset extensions in the config file and instead just have the smallest granularity extensions. These should still exist in the extensions enum and would get turned on automatically if all of the appropriate subset extensions are enabled.

In terms of what to print in the device tree/isa string, it might be the least ambiguous to just print the full list even if that is more verbose. I know some tools (like riscv-config) don't always handle the superset extensions well.

@pmundkur
Copy link
Collaborator

Yes, I think for now we should stick to the smallest granularity extensions in the model and config file. We could rely on other tooling (such as that in #993) to convert superset extensions into Sail model configurations.

@nadime15 nadime15 force-pushed the add_zvk_extensions branch 2 times, most recently from 792604f to 582d223 Compare June 25, 2025 20:09
@Timmmm
Copy link
Collaborator

Timmmm commented Jun 25, 2025

But note that in some cases it isn't as simple as extension 1 = extensions 2 + 3. IIRC think supporting B or C is different to supporting the extensions that they cover. In this case I believe it is though.

@nadime15 nadime15 changed the title Add config flags for remaining Vector Crypto (Zvk*) extensions Auto-enable Vector Crypto superset extensions (Zvkn, Zvknc, Zvkng, Zvks, Zvksc, Zvksg) Jun 25, 2025
@nadime15 nadime15 force-pushed the add_zvk_extensions branch from f39f46a to d35ae3b Compare June 26, 2025 13:11
@jordancarlin
Copy link
Collaborator

hartSupports_measure in riscv_termination.sail needs to be updated with the dependency order to fix the Lean build.

@nadime15 nadime15 force-pushed the add_zvk_extensions branch from d35ae3b to 7f94a23 Compare June 26, 2025 17:09
@pmundkur
Copy link
Collaborator

I'm rethinking the choice to not expose superset extensions in the config file. It necessitates extra tooling (and a degree of user-unfriendliness); is there a way of handling this without extra tooling?

This would mean some extensions may not appear in the config file, e.g. if Zvknc is marked as supported, Zvkn and Zvbc may not appear since they would be redundant. This would require some additional flexibility in the configuration mechanism, e.g. a check for whether a configuration option appears before trying to read its value.

@pmundkur pmundkur added the tgmm-agenda Tagged for the next Golden Model meeting agenda. label Jun 27, 2025
@nadime15 nadime15 force-pushed the add_zvk_extensions branch from 7f94a23 to 4652341 Compare June 30, 2025 21:13
@nadime15 nadime15 force-pushed the add_zvk_extensions branch from 4652341 to 96ba8c3 Compare July 2, 2025 17:02
Copy link
Collaborator

@jordancarlin jordancarlin left a comment

Choose a reason for hiding this comment

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

LGTM based on the decisions made in Monday's meeting.

@Timmmm
Copy link
Collaborator

Timmmm commented Jul 3, 2025

Isn't this going to put the superset extensions and the subset ones in the ISA string? That seems slightly odd... Or is that intentional?

@jordancarlin
Copy link
Collaborator

Isn't this going to put the superset extensions and the subset ones in the ISA string? That seems slightly odd... Or is that intentional?

I think we decided that the generated ISA string should take the approach of being exhaustive to satisfy tools that might be looking for one or the other.

@pmundkur pmundkur added the will be merged Scheduled to be merged soon if nobody objects label Jul 3, 2025
@jordancarlin jordancarlin removed the tgmm-agenda Tagged for the next Golden Model meeting agenda. label Jul 4, 2025
@Timmmm Timmmm added this pull request to the merge queue Jul 4, 2025
Merged via the queue into riscv:master with commit a0c0741 Jul 4, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

will be merged Scheduled to be merged soon if nobody objects

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants