Make extension discriminators clearly distinguish their purpose

[ThinkOpenly]

Currently, fairly innocuous function names are used to mark an instruction as part of an extension:

mapping clause encdec = RISCV_FMINM_S(rs2, rs1, rd) if haveZfa()

A parser would have to know that a function name like "have" might* distinguish an extension, but there are no guarantees.

One suggested solution:

mapping clause encdec = RISCV_FMINM_S(rs2, rs1, rd) if extension("Zfa")

At least using a consistent function name establishes a namespace in which extensions can be enumerated, something like:

val extension : (string) -> bool

function extension(ext) = {
  match ext {
    "Atomics" => misa.A() == 0b1,
    "Zfa" => true,
    "Zfh" => (misa.F() == 0b1) & (mstatus.FS() != 0b00),
    "Zicond" => true,
    _ => false
  }
}

[Bakugo90]

Hello @ThinkOpenly , I beginner to open-source and want to contribute on solving this issues. Please can you help me well understand this issues ? I must rename extension and function name in order to use a consistent name that cleary show their purpose, right ?

If i'm right, in which file of "models/" directory i can do the task ?

[ThinkOpenly]

I must rename extension and function name in order to use a consistent name that cleary show their purpose, right ?

There are some layers in this issue.

The first, and most straightforward changess suggested by this issue are to turn instances like this:

mapping clause encdec = RISCV_FMINM_S(rs2, rs1, rd) if haveZfa()

into this:

mapping clause encdec = RISCV_FMINM_S(rs2, rs1, rd) if extension("Zfa")

and then adding that extension string ("Zfa" in this case) to the new function extension as suggested in the original issue description.

A second, and more difficult layer, is to find places where the instructions for other extensions are defined, but not even marked as extensions at all. All of the instructions in the "M" extension are like this for one prominent example, such as:

mapping clause encdec = MUL(rs2, rs1, rd, high, signed1, signed2)

If i'm right, in which file of "models/" directory i can do the task ?

That is the right place. For the first layer, though, I have already implemented all of the necessary changes. Look at the "JSON" branch in this repository. Further work here should probably be on top of that branch.

For the second layer, it is convenient that the instructions are divided up into different files by extension. So, pretty much all instructions in files like riscv_insts_*.sail which are not marked as extensions should be.

[jriyyya]

For the second layer, it is convenient that the instructions are divided up into different files by extension. So, pretty much all instructions in files like riscv_insts_*.sail which are not marked as extensions should be.

Hi @ThinkOpenly , I was wondering if this is applicable to #1 and #2 as well. The names and descriptions are to be added to all riscv_insts_*.sail files?

[ThinkOpenly]

I was wondering if this is applicable to #1 and #2 as well. The names and descriptions are to be added to all riscv_insts_*.sail files?

Yes, but do read https://github.com/ThinkOpenly/sail-riscv/pull/7#issuecomment-1900720496. Adding names and descriptions is not as universally easy as I had originally envisioned.

[Sanket-0510]

The PR #8 do addresses the First layer as described by @ThinkOpenly but fails to implement Second layer which is to having a function extension to cover all the extensions defined by the "base ISA" which includes extensions like A,D,I, V,etc. Then using this function to cover all the extension related checks. I am not sure if I am correct or not but we will need to define helpers functions like this put it in a file name riscv_insts_helpers.sail and make use of them for checking the extensions

[Bakugo90]

Greetings, @Sanket-0510, I just saw that you implemented the "C" and "A" extension. For "C" . You did a great job because I had some difficulty understanding it.

But for extension "A", I have already done it. I noticed that you did it again in your PR.

Also the function you defined in the riscv_insts_helpers.sail file already exists in the riscv_sys_regs.sail file. Can you explain a little more why you think we need to create another file for this?

[Sanket-0510]

Greetings, @Sanket-0510, I just saw that you implemented the "C" and "A" extension. For "C" . You did a great job because I had some difficulty understanding it.

But for extension "A", I have already done it. I noticed that you did it again in your PR.

Also the function you defined in the riscv_insts_helpers.sail file already exists in the riscv_sys_regs.sail file. Can you explain a little more why you think we need to create another file for this?

Could you please highlight the part of the code where this function has already been implemented? It seems that I may have overlooked it. I did it again for extension "A" cause I felt that you are missing onto the required function.

[Sanket-0510]

Greetings, @Sanket-0510, I just saw that you implemented the "C" and "A" extension. For "C" . You did a great job because I had some difficulty understanding it.

But for extension "A", I have already done it. I noticed that you did it again in your PR.

Also the function you defined in the riscv_insts_helpers.sail file already exists in the riscv_sys_regs.sail file. Can you explain a little more why you think we need to create another file for this?

I was working from the main branch reference; hence, I missed the extension function in the riscv_sys_regs.sail file which @ThinkOpenly has already has defined on the JSON branch.

[ThinkOpenly]

I think commit 7dfb21f addresses the remaining bits of this issue sufficiently, in that there are no more have* functions, all being replaced by calls to extension(). The explicit use of extension("A"), for example, arguably makes the purpose clear where used. Closing this issue... Thanks to all the contributors!