new mechanism for reserved fields as mentioned in Issue #21 #24

joydeep049 · 2024-05-12T05:52:12Z

closes #21
Used the same mask as defined in binutils to make sure only the essential bits are retrieved while the others are zeroed out.
Please review @ThinkOpenly

ThinkOpenly

I see that you have added a "doc comment" including text from the specification, and a mask for the bitfield.

Tell me a more about the problem you are attempting to solve.

The "doc comments" in this context are intended to be human-readable descriptions of the instruction semantics. See

sail-riscv/model/riscv_insts_base.sail

Line 250 in ffaadb0

/*!

. The text you added is discussing just the optional fields. (Important information in this context, but incomplete.)

I'll note that the mask you added has no true effect, as the value with and without the mask is the same.

Issue #21 is asking for some sort of mechanism in the Sail code to identify which fields are reserved. It could be something simple like changing 0b00000 to reserved(0b00000), where reserved is implemented as an identity function. This would allow the code to be compiled, yet the parser would produce an AST with a "reserved" tag of sorts.

joydeep049 · 2024-05-13T17:38:53Z

Thank you for the review! @ThinkOpenly
As per my understanding, in this issue we basically had to "let everyone know" that even though the imm,rs1 and rd fields are zeroed out currently, we might use them later.
I was not sure how exactly to implement this.
Two ways came to mind:

Simple - Add comments and let people know, hence the use of the doc comments.
Do something similar to what binutils did and mask it. It seems this idea was mistaken and therefore doesn't yield any results.

ThinkOpenly · 2024-05-14T17:55:08Z

As per my understanding, in this issue we basically had to "let everyone know" that even though the imm,rs1 and rd fields are zeroed out currently, we might use them later. I was not sure how exactly to implement this. Two ways came to mind:

Simple - Add comments and let people know, hence the use of the doc comments.

Do something similar to what binutils did and mask it. It seems this idea was mistaken and therefore doesn't yields any results.

The goal with the project is to formalize what's currently in the Sail code into a more useful format. I've chosen JSON. So, there needs to be a clear means to translate important aspects found in the Sail code into a JSON representation. In the case under discussion, certain fields are reserved (and not constant as the Sail code currently represents).

The means I suggested in my last comment was to add an annotation to the fields, like reserved(0b00000). This is an ad-hoc annotation, but at least serves our purposes in a fairly straightforward and Sail-compatible way. What is needed, though, is a definition of the reserved function, even if it is just an identity function. Unfortunately, Sail's reasonably strong typing might force us to create an identity function for every field length:

function reserved_bits_5 f : bits(5) -> bits(5) = f

Looking a bit deeper, it looks like the Sail code has kind of made a mess of this. It does represent the "reserved" case of the fence.i instruction, but it does it as a new (invalid) mnemonic, fence.i.reserved, in a different file (model/riscv_insts_hints.sail):

mapping clause assembly = FENCEI_RESERVED(imm, rs, rd)
      if imm != 0b000000000000 | rs != zreg | rd != zreg
  <-> "fence.i.reserved." ^ reg_name(rd) ^ "." ^ reg_name(rs) ^ "." ^ hex_bits_12(imm)
      if imm != 0b000000000000 | rs != zreg | rd != zreg

sigh. Strange choices. Sail does not offer a ready way to handle this, though. This needs some further thought.

joydeep049 · 2024-05-15T07:53:58Z

@ThinkOpenly You're right. Reserved fences are being used here in (model/riscv_insts_hints.sail), but I think the way we want to use it here is a bit different. And we cannot possibly go about creating identity functions for every bit length.

How should we approach our current situation here? Any ideas and suggestions are welcome.

Thanx!

ThinkOpenly · 2024-05-16T15:31:33Z

Let's move the general discussion to the associated issue #21. I've already added a comment there.

joydeep049 · 2024-05-17T17:33:03Z

Hello @ThinkOpenly , I've made the changes as discussed in here.
Could you tell me more on how I could Modify the Sail JSON backend to recognize functions with names like reserved_bits_N, where N is the bit length.

ThinkOpenly · 2024-05-22T17:51:48Z

Could you tell me more on how I could Modify the Sail JSON backend to recognize functions with names like reserved_bits_N, where N is the bit length.

I put a response in issue #21.

joydeep049 · 2024-05-23T10:24:28Z

Could you tell me more on how I could Modify the Sail JSON backend to recognize functions with names like reserved_bits_N, where N is the bit length.

I put a response in issue #21.

I'm working on this . Apologies for the late reply (was travelling since last 2 days).

Signed-off-by: Paul A. Clarke <[email protected]>

Produces JSON representation of instructions, operands, opcode layouts, etc. Not claimed to be exhaustive. Depends on https://github.com/ThinkOpenly/riscvdecode. Signed-off-by: Paul A. Clarke <[email protected]>

…istently

More support for ThinkOpenly#4. Using the `extension()` function in `mapping clause encdec` expressions for extensions allows a parser to clearly know when a function is part of an extension (or set of extensions).

…e.sail Added instruction names and descriptions * Adds names for instructions in `riscv_insts_base.sail` using the attributes approach * Descriptions are added to instructions in the same file using the doc comments approach

Currently, when executing `make json`, the command itself is echoed, causing issues with the JSON representation. This PR Suppress this echoing, ensuring a proper JSON output without the need for any manual editing later on.

Some instructions are grouped in a single `mapping clause assembly` where the respective mnemonics are embedded within a separate `mapping`. For instructions which are _not_ grouped, the name of the instruction can be added as an attribute to any of the instruction-specific constructs. For grouped instructions, the attribute needs to be added to a construct at the granularity of the specific instruction. Here, we attach the attribute within the individual element of the `mapping` that includes the actual instruction mnemonic. This approach is still insufficient for mnemonics that are constructed: ``` mapping clause assembly = VLSEGTYPE(nf, vm, rs1, width, vd) <-> "vl" ^ nfields_string(nf) ^ "e" ^ vlewidth_bitsnumberstr(width) ^ ".v" [...] ``` ...so more thought is needed here. Also, I'm settling on a convention of using lower case (not Leading Caps), at least for now. This matches how the instruction names are written in the ISA specification, at least for those instructions for which the name is actually provided. :-/

* Move definition of function of `extension` * Utilize extension() instead of `haveZmmul()` * Utilize extension() instead of `haveUsrMode()` * Utilize extension() instead of `haveSupMode()` * Utilize extension() instead of `haveNExt()` * Utilize extension() instead of `haveZkr()` * Utilize extension() instead of `haveUsrMode()` * Move comments from `have*` functions into `extension` function * Delete all unused `have*` definitions of various extensions Fixes ThinkOpenly#4 .

Authored-by: Joydeep Tripathy <[email protected]>

GitHub CI is complaining: ``` fix end of files.....................Failed - hook id: end-of-file-fixer - exit code: 1 - files were modified by this hook Fixing doc/JSON.md [...] All changes made by hooks: diff --git a/doc/JSON.md b/doc/JSON.md index 82170ef..fc7f9cc 100644 [...] -4. Within that clone : `make json` \ No newline at end of file +4. Within that clone : `make json` Error: Process completed with exit code 1. ```

ThinkOpenly · 2024-07-29T14:47:27Z

function reserved_bits_5 () = 0b00000

Using this method gave the following error:

Type error:
model/riscv_types.sail:413.0-37:
413 |function reserved_bits_5 () = 0b00000
    |^-----------------------------------^
    | Cannot infer function type for unannotated function
make: *** [Makefile:242: generated_definitions/ocaml/RV64/riscv.ml] Error 1

Maybe it should provide the input-output types (not tested):

function reserved_bits_5 () : unit -> bits(5) = 0b00000

joydeep049 · 2024-07-29T14:58:37Z

function reserved_bits_5 () = 0b00000

Using this method gave the following error:

Type error:
model/riscv_types.sail:413.0-37:
413 |function reserved_bits_5 () = 0b00000
    |^-----------------------------------^
    | Cannot infer function type for unannotated function
make: *** [Makefile:242: generated_definitions/ocaml/RV64/riscv.ml] Error 1

Maybe it should provide the input-output types (not tested):

function reserved_bits_5 () : unit -> bits(5) = 0b00000

Type error:
model/riscv_insts_base.sail:791.27-44:
791 |  <-> reserved_bits_12() @ reserved_bits_5() @ 0b001 @ reserved_bits_5() @ 0b0001111
    |                           ^---------------^
    | Cannot infer width here, as there are multiple subpatterns with unclear width in vector concatenation pattern
    | 
    | Caused by model/riscv_insts_base.sail:791.6-24:
    | 791 |  <-> reserved_bits_12() @ reserved_bits_5() @ 0b001 @ reserved_bits_5() @ 0b0001111
    |     |      ^----------------^
    |     | A previous subpattern is here
make: *** [Makefile:242: generated_definitions/ocaml/RV64/riscv.ml] Error 1

Getting the same error as before. I did try this before as well by reading from the sail manual but nothing else worked.
@ThinkOpenly

ThinkOpenly · 2024-07-29T15:18:31Z

Getting the same error as before.

Not that I don't trust you, but could you show the code you added for both reserved_bits_5 and reserved_bits_12 here, and where you added them? Sail does seem to have trouble inferring widths for some things that have very obvious widths, which is quite disappointing. Let's verify your code first.

joydeep049 · 2024-07-29T15:24:36Z

Getting the same error as before.

Not that I don't trust you, but could you show the code you added for both reserved_bits_5 and reserved_bits_12 here, and where you added them? Sail does seem to have trouble inferring widths for some things that have very obvious widths, which is quite disappointing. Let's verify your code first.

function reserved_bits_12 () : unit -> bits(12) = 0b000000000000
function reserved_bits_5 () : unit -> bits(5) = 0b00000

This is the code. 🥲🥲

ThinkOpenly · 2024-07-29T17:55:25Z

Getting the same error as before.

Not that I don't trust you, but could you show the code you added for both reserved_bits_5 and reserved_bits_12 here, and where you added them? Sail does seem to have trouble inferring widths for some things that have very obvious widths, which is quite disappointing. Let's verify your code first.
function reserved_bits_12 () : unit -> bits(12) = 0b000000000000
function reserved_bits_5 () : unit -> bits(5) = 0b00000
This is the code. 🥲🥲

I opened an issue with upstream Sail to try to clarify the restrictions we seem to be running into with bidirectional mapping clauses. rems-project/sail#651. It appears that it may not be possible to use functions in these clauses, somewhat surprisingly to me.

Using what might be considered a workaround (or a hack!), maybe try something like (tested lightly):

enum reserved_bits_enum = { ZERO }
mapping reserved_bits_5 : reserved_bits_enum <-> bits(5) = { ZERO <-> 0b00000 }
mapping reserved_bits_12 : reserved_bits_enum <-> bits(12) = { ZERO <-> 0b000000000000 }

mapping clause encdec = FENCEI()
  <-> reserved_bits_12(ZERO) @ reserved_bits_5(ZERO) @ 0b001 @ reserved_bits_5(ZERO) @ 0b0001111

There may be less ugly ways of doing the same.

ThinkOpenly · 2024-07-29T18:29:53Z

There may be less ugly ways of doing the same.

Possibly slightly less ugly:

enum reserved_bits_enum = { RESERVED }
mapping bits_5 : reserved_bits_enum <-> bits(5) = { RESERVED <-> 0b00000 }
mapping bits_12 : reserved_bits_enum <-> bits(12) = { RESERVED <-> 0b000000000000 }
mapping size_bits : word_width <-> bits(12) = {
  BYTE   <-> bits_12(RESERVED),
  HALF   <-> bits_5(RESERVED) @ bits_5(RESERVED) @ 0b01,
[...]

joydeep049 · 2024-07-30T08:47:22Z

I have made the suggested changes and they seem to work fine.
@ThinkOpenly

(Reminder to approve the workflows in the sail repository. We might have to make some changes to the OCaml code.)

ThinkOpenly · 2024-07-30T22:41:35Z

I have made the suggested changes and they seem to work fine.

🎉

(Reminder to approve the workflows in the sail repository. We might have to make some changes to the OCaml code.)

Do I? The actions ran (successfully, with a couple of warnings about using an outdated version of Node.js). What changes to the OCaml code?

You still need to rebase.

joydeep049 · 2024-07-31T06:43:44Z

I meant my PR in `ThinkOpenly/sail'. @ThinkOpenly

joydeep049 · 2024-08-09T06:42:12Z

Should we merge this? @ThinkOpenly

ThinkOpenly

One final change, please.

model/riscv_insts_base.sail

ThinkOpenly · 2024-08-15T17:51:40Z

Now that we have good code, we need to clean up the branch before merging. You should rebase against the target branch (https://github.com/ThinkOpenly/sail-riscv/tree/JSON), since you seem to have a commit from jriyyya lumped into your PR that you reverted with yet another commit. Also, given the net changes are pretty small, could you squash your commits into a single commit? (Or, I can do it when merging.)

joydeep049 · 2024-08-16T10:20:11Z

My git client misbehaved while I was rebasing after squashing.
Should I do it again or is this again?
Also seems like it did not accept my final commit message.
@ThinkOpenly

ThinkOpenly · 2024-08-16T22:17:13Z

This is still in a pretty weird state. I expect a single commit. Currently, there are 3 in this PR:

one from jriyyya (a rebase to the target branch should fix this)
one "initial commit" that I think should just be removed
one "address failing tests" that has the 6 or 7 new lines that are the change, but also includes reversions of the previous 2 commits.

Also seems like it did not accept my final commit message.

Yeah, the commit message in the final commit is a bit of a mess.

If you need help with git, let me know.

joydeep049 · 2024-08-17T18:10:59Z

Hello @ThinkOpenly

I have squashed all into a single commit.
Hope this works.

Additionally, I wanted to inform you that I've submitted my LFX application. Looking forward to your feedback on both the application and this PR.

ThinkOpenly · 2024-08-20T16:31:38Z

I have squashed all into a single commit. Hope this works.

Well... getting closer. The single commit has Riya as the author, and you as the committer:

Can you fix that?

ThinkOpenly reviewed May 13, 2024

View reviewed changes

ThinkOpenly mentioned this pull request May 14, 2024

Need a new mechanism for reserved fields #21

Open

ThinkOpenly and others added 21 commits June 19, 2024 07:43

Assembly for hint instructions needs spaces after mnemonics

f26d17d

Signed-off-by: Paul A. Clarke <[email protected]>

Add json target

11a9f93

Produces JSON representation of instructions, operands, opcode layouts, etc. Not claimed to be exhaustive. Depends on https://github.com/ThinkOpenly/riscvdecode. Signed-off-by: Paul A. Clarke <[email protected]>

Add "extension" function to facilitate parsing extension content cons…

66ca42a

…istently

WIP

83b7f54

support most remaining extensions via extension()

d2e3009

hints again

4adc0a6

extensions "V" and "Zbkb"

1e6b213

Add C extension conditionals

4746476

Use extension discriminator for "A" and "M" extensions

7596938

More support for ThinkOpenly#4. Using the `extension()` function in `mapping clause encdec` expressions for extensions allows a parser to clearly know when a function is part of an extension (or set of extensions).

Fix up some whitespace and capitalization

8873845

[JSON] Suppress command echo to stdout

294b661

Currently, when executing `make json`, the command itself is echoed, causing issues with the JSON representation. This PR Suppress this echoing, ensuring a proper JSON output without the need for any manual editing later on.

Utilize function extension instead of haveZdinx()

316405f

Utilize extension function instead of haveZhinx()

ef956d4

Utilize extension function instead of haveZfinx()

8af82b3

Add compilation steps to README.md

b4a1d40

Authored-by: Joydeep Tripathy <[email protected]>

Add names for SHA2-256 instructions

f381848

Add names for SHA2-512 instructions

f460671

joydeep049 force-pushed the issue-21 branch from 3e39f44 to ccf8c7e Compare July 31, 2024 10:13

Linda-Njau added 2 commits August 7, 2024 12:51

Add names for base instructions

f1b2fc8

Add names for cdext instructions

d726e29

ThinkOpenly requested changes Aug 14, 2024

View reviewed changes

model/riscv_insts_base.sail Outdated Show resolved Hide resolved

Linda-Njau added 7 commits August 15, 2024 14:01

Add names for cext instructions

adbd4ab

Fix case for Zba instruction names

7c2b7c1

Fix case for Zbb instruction names

ef71e7b

Fix case for Zbc instruction names

079c774

Fix case for Zbkx instruction names

bbcd289

Fix case for Zbkb instruction names

0000e79

Fix case for Zbs instruction names

5c3aad8

joydeep049 force-pushed the issue-21 branch from 0ace95d to 0a37d92 Compare August 16, 2024 10:18

feat: added reserved fields

6fa0840

joydeep049 force-pushed the issue-21 branch from 0a37d92 to 6fa0840 Compare August 17, 2024 18:09

ThinkOpenly force-pushed the JSON branch from 12267d1 to 91fb454 Compare September 16, 2024 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new mechanism for reserved fields as mentioned in Issue #21 #24

new mechanism for reserved fields as mentioned in Issue #21 #24

joydeep049 commented May 12, 2024 •

edited

Loading

ThinkOpenly left a comment

joydeep049 commented May 13, 2024 •

edited

Loading

ThinkOpenly commented May 14, 2024

joydeep049 commented May 15, 2024

ThinkOpenly commented May 16, 2024

joydeep049 commented May 17, 2024

ThinkOpenly commented May 22, 2024

joydeep049 commented May 23, 2024

ThinkOpenly commented Jul 29, 2024

joydeep049 commented Jul 29, 2024

ThinkOpenly commented Jul 29, 2024

joydeep049 commented Jul 29, 2024

ThinkOpenly commented Jul 29, 2024

ThinkOpenly commented Jul 29, 2024

joydeep049 commented Jul 30, 2024

ThinkOpenly commented Jul 30, 2024

joydeep049 commented Jul 31, 2024

joydeep049 commented Aug 9, 2024

ThinkOpenly left a comment

ThinkOpenly commented Aug 15, 2024

joydeep049 commented Aug 16, 2024 •

edited

Loading

ThinkOpenly commented Aug 16, 2024

joydeep049 commented Aug 17, 2024 •

edited

Loading

ThinkOpenly commented Aug 20, 2024

new mechanism for reserved fields as mentioned in Issue #21 #24

Are you sure you want to change the base?

new mechanism for reserved fields as mentioned in Issue #21 #24

Conversation

joydeep049 commented May 12, 2024 • edited Loading

ThinkOpenly left a comment

Choose a reason for hiding this comment

joydeep049 commented May 13, 2024 • edited Loading

ThinkOpenly commented May 14, 2024

joydeep049 commented May 15, 2024

ThinkOpenly commented May 16, 2024

joydeep049 commented May 17, 2024

ThinkOpenly commented May 22, 2024

joydeep049 commented May 23, 2024

ThinkOpenly commented Jul 29, 2024

joydeep049 commented Jul 29, 2024

ThinkOpenly commented Jul 29, 2024

joydeep049 commented Jul 29, 2024

ThinkOpenly commented Jul 29, 2024

ThinkOpenly commented Jul 29, 2024

joydeep049 commented Jul 30, 2024

ThinkOpenly commented Jul 30, 2024

joydeep049 commented Jul 31, 2024

joydeep049 commented Aug 9, 2024

ThinkOpenly left a comment

Choose a reason for hiding this comment

ThinkOpenly commented Aug 15, 2024

joydeep049 commented Aug 16, 2024 • edited Loading

ThinkOpenly commented Aug 16, 2024

joydeep049 commented Aug 17, 2024 • edited Loading

ThinkOpenly commented Aug 20, 2024

joydeep049 commented May 12, 2024 •

edited

Loading

joydeep049 commented May 13, 2024 •

edited

Loading

joydeep049 commented Aug 16, 2024 •

edited

Loading

joydeep049 commented Aug 17, 2024 •

edited

Loading