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

[vector-crypto] Fixing Zvkb/Zvbb distinction #1474

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

Conversation

nibrunieAtSi5
Copy link
Contributor

It looks like vector crypto support introduced in #1303 was made before the Zvkb was re-introduced as a subset of Zvbb.

Working on fixing this here.

riscv/riscv.mk.in Outdated Show resolved Hide resolved
disasm/isa_parser.cc Outdated Show resolved Hide resolved
disasm/disasm.cc Outdated
@@ -2233,38 +2233,43 @@ void disassembler_t::add_instructions(const isa_parser_t* isa)
DEFINE_R1TYPE(sm3p1);
}

if (isa->extension_enabled(EXT_ZVBB)) {
if (isa->extension_enabled(EXT_ZVKB) || isa->extension_enabled(EXT_ZVBB)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is the accepted way to condition instructions shared between two extensions.

Copy link
Collaborator

@aswaterman aswaterman Oct 2, 2023

Choose a reason for hiding this comment

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

You only need to check for Zvkb because of this line, which guarantees that if Zvbb is present then Zvkb is also present.

https://github.com/riscv-software-src/riscv-isa-sim/pull/1474/files#diff-28b412bc112a3a620c001f69229c74da5d1cdf37a0293562e9209ee93c6dff04R240

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I should have read the code I changed ...

disasm/disasm.cc Outdated Show resolved Hide resolved
@dybv-sc
Copy link

dybv-sc commented Jul 3, 2024

@nibrunieAtSi5 , @nibrunie Hi, are you still going to finish this? This feature would be really nice to have, and I was going to implement it myself, but then I noticed your commit.

@nibrunie
Copy link

nibrunie commented Jul 7, 2024

@nibrunieAtSi5 , @nibrunie Hi, are you still going to finish this? This feature would be really nice to have, and I was going to implement it myself, but then I noticed your commit.

Yes I need to revive this. I will try to get it up for review this week (July 8th 2024).

@nibrunieAtSi5 nibrunieAtSi5 marked this pull request as ready for review July 22, 2024 20:00
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.

4 participants