Add a consistent mechanism for identifying instruction's containing extension

[ThinkOpenly]

A new mechanism is introduced to more consistently identify which instruction content is contained within which RISC-V ISA extensions.

The current implementations use somewhat arbitrary functions with names of the form "have*", which are not consistently named, and not always obviously associated with a specific RISC-V extension, examples:

• haveFExt: "F" extension
• haveZfinx: "Zfinx" extension
• haveRVC: "C" extension
• haveSupMode: "S" extension

The new mechanism uses a new function, extension, which takes an extension identifier string, uses the content of the existing "have*" functions for the specific extension of the given name, and produces a boolean result -- one-stop-shopping to determine the status of extension support. The above "have*" function examples are replaced by:

• extension("F")
• extension("Zfinx")
• extension("C")
• extension("S")

In addition, there are some instructions with content that is not tagged. Those missing tags have been added.

A not insignificant benefit is that this important content of the Sail code becomes more consistently parsable/actionable by Sail parser backend code.

[github-actions]

Test Results

712 tests  ±0   712 ✅ ±0   0s ⏱️ ±0s
  6 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 3d5e476. ± Comparison against base commit db5f430.

♻️ This comment has been updated with latest results.

[jrtc27]

Per #495 (comment), please use an enum and scatter both the enum and function(s).

[Alasdair]

I was thinking about this a bit recently, and I'm not sure an enum is the way to go. The problem with an enum is it adds all the names to the global identifier namespace, which for RISC-V extensions means you end up single character identifiers like A, C, N, etc all being globally defined, unless you go for some prefixing scheme like Extension_A, Extension_Zfinx, etc.

I fairly recently added some (experimental) support for a string_literal type in Sail which is a subtype of the string type. The advantage there is if you know a string is a literal because of its type, you can compile it into an enumeration which solves some of the tricky string-related issues for generating SystemVerilog and SMT from Sail.

[Alasdair]

Other than the question of whether extension names should be string literals or elements of an enum, this is 100% what we should be doing though.

[jrtc27]

Strings have the ability to be mis-typed and go undetected at compile time as a result though, and require a wildcard _ => false (or assert) case.

[Alasdair]

Yes, that is definitely a major point in favour of using an enumeration.

[Timmmm]

Yeah I think a scattered enum with a prefix is the best option. I would really like if all Sail enums were namespaced tbh (enum class? :-D), but using a prefix as a poor-mans namespace is decent enough.

Also I think just calling the function extension is not at all descriptive. How about:

... if extension_enabled(Ext_F)
... if extension_enabled(Ext_Zfinx)

[ThinkOpenly]

@jrtc27 , @Alasdair , @Timmmm : how does this look as a prototype (passes make check)?

diff --git a/model/riscv_insts_aext.sail b/model/riscv_insts_aext.sail
index f7ef3c8469b4..d44bfd262eb9 100644
--- a/model/riscv_insts_aext.sail
+++ b/model/riscv_insts_aext.sail
@@ -14,0 +15,3 @@
+enum clause extensions = Extension_A
+function clause extension_supported(Extension_A) = misa[A] == 0b1
+
diff --git a/model/riscv_insts_begin.sail b/model/riscv_insts_begin.sail
index 4287535c00b6..09e55d6b509c 100644
--- a/model/riscv_insts_begin.sail
+++ b/model/riscv_insts_begin.sail
@@ -28,0 +29,5 @@ scattered mapping encdec_compressed
+scattered enum extensions
+
+val extension_supported : extensions -> bool
+scattered function extension_supported
+
diff --git a/model/riscv_insts_end.sail b/model/riscv_insts_end.sail
index d6a328d4844a..3e0c2a3ec3f1 100644
--- a/model/riscv_insts_end.sail
+++ b/model/riscv_insts_end.sail
@@ -29,0 +30,2 @@ mapping clause assembly = C_ILLEGAL(s) <-> "c.illegal" ^ spc() ^ hex_bits_16(s)
+end extension_supported
+end extensions

[jrtc27]

Supported is ambiguous, could be either "can be enabled" or "is enabled", and I'd abbreviate to something like Ext_ to make it less clumsy, but otherwise that looks like what I was imagining.

[Timmmm]

I agree with Jessica - see here. Let's stick with "enabled" for "it's implemented and its use is allowed by misa" and maybe "implemented" for "the CPU has hardware for this extension" (not needed in this PR but I think we should maybe add an extension_implemented(Ext_...) function in future to replace the sys_... callbacks.

[ThinkOpenly]

I've changed everything to:

• scattered enums (extensions) for the extension identifiers
• scattered functions (extensionEnabled) for what was 'have' functions

Let me know if you have any further comments.

[rmn30]

Thanks for this PR, it is a great improvement! I agree that we could do with ways to distinguish implemented, enabled, enabled / disabled on reset and 'hardwired to on' as these affect misa initialisation and legalization.

[Timmmm]

I agree this is a great improvement. LGTM except some very minor nits.

[ThinkOpenly]

rebased to address conflicts with recent commit 4817b64

[ThinkOpenly]

@billmcspadden-riscv Anything else needed here before merging? Thanks!

[jrtc27]

Hang on, nobody approved the version you merged, and the issue I pointed out wasn't properly resolved. The line that was wrong got deleted, not changed to use the right scattered definition name.

[ThinkOpenly]

Hang on, nobody approved the version you merged, and the issue I pointed out wasn't properly resolved. The line that was wrong got deleted, not changed to use the right scattered definition name.

Sorry, the one-word review "Stale" left too much to my imagination, and I mistakenly thought I'd left an old reference in the code, not that I'd failed to rename it.

I can submit a new PR with that renamed end extensionEnabled added back in. Is there anything else that would be required to get closure on this PR?

[jrtc27]

I think that’s the only thing that needs fixing. It’s not like it’s a big deal, but it’s an instance of how this repository continues to be poorly managed, with a lack of diligence by those responsible, and it concerns me that we’re still having conversations about how to properly maintain an open source project.

[ThinkOpenly]

Part of the story not seen here is that this PR was discussed in some detail at the "Golden Model" meeting, 2024-07-15, and approved for merging by several participants (including, if I'm not mistaken, @Timmmm , @Alasdair ).

I will submit a new PR with the suggested correction shortly.

[jrtc27]

I mean, it's customary to check that the people who left review comments are happy before merging...

Plus, the fact that you discussed it at the golden model meeting does not notify anyone following GitHub, nor does it give an obvious trail for people to follow on GitHub for who actually reviewed and approved the changes, and which exact version they reviewed/approved.

[ThinkOpenly]

I mean, it's customary to check that the people who left review comments are happy before merging...

Your review comments arrived after the approvals that occurred at the meeting, but yes, a glance at the actual PR would've obviously shown the new unaddressed comments before the merge.

Plus, the fact that you discussed it at the golden model meeting does not notify anyone following GitHub, nor does it give an obvious trail for people to follow on GitHub for who actually reviewed and approved the changes, and which exact version they reviewed/approved.

Agree that this process could be approved. For PRs (and issues) that are discussed in the meeting, some relevant notes should be relayed into the PR/issue. @billmcspadden-riscv , thoughts?

[billmcspadden-riscv]

Definitely my fault. No excuses. I did not read the PR thoroughly and based the promotion on the conversation we had at the tgmm. I will be more thorough. FYI: My general rules when promoting a PR: 1. Have at least 2 reviews of the PR, one of which is from an SME. 2. Ensure that all github conversations in the code review are closed and are closed by the initiator of the conversation. 3. Ensure that there are no merge conflicts. Bill Mc.

…

On Mon, Jul 22, 2024 at 7:21 PM Paul Clarke ***@***.***> wrote: I mean, it's customary to check that the people who left review comments are happy before merging... Your review comments arrived after the approvals that occurred at the meeting, but yes, a glance at the actual PR would've obviously shown the new unaddressed comments before the merge. Plus, the fact that you discussed it at the golden model meeting does not notify anyone following GitHub, nor does it give an obvious trail for people to follow on GitHub for *who* actually reviewed and approved the changes, and which exact version they reviewed/approved. Agree that this process could be approved. For PRs (and issues) that are discussed in the meeting, some relevant notes should be relayed into the PR/issue. @billmcspadden-riscv <https://github.com/billmcspadden-riscv> , thoughts? — Reply to this email directly, view it on GitHub <#506 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AXROLOERJW2FYZ6CJERZIOLZNWOW3AVCNFSM6AAAAABKJQSTL2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBUGAZDQOJUGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Bill McSpadden Formal Verification Engineer RISC-V International mobile: 503-807-9309 Join me at RISC-V Summit North America <http://www.riscvsummit.com/> in October!