search

ThinkOpenly / sail-riscv

Sail RISC-V model
Other
12 stars 14 forks source link

Added names and descriptions for instructions in riscv_insts_base.sail #7

Closed jriyyya closed 7 months ago

jriyyya commented 9 months ago

Implements #1 and #2

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

jriyyya commented 9 months ago

Hi @ThinkOpenly, I looked at the recommended approaches you left in the comments of these issues. I tried to implement this, please let me know if the approach is correct. If this is fine I will also try to implement Alasdair Armstrong's approach for #3

ThinkOpenly commented 9 months ago

I looked at the recommended approaches you left in the comments of these issues. I tried to implement this, please let me know if the approach is correct.

You have indeed followed the approach I recommended, but you have also exposed my approach as naive. (Not your fault, surely.) The problem is the instructions which are grouped under a single union clause ast. Especially for the instruction name, we need a way to associate, for example, "beq" with the name "Branch if equal". This also leads to the descriptions being overly general, such that "beq" includes the descriptions for "beq", "bne", "blt", "bge", "bltu", and "bgeu". This is a structural issue in the ISA document. I know there is work being done there, but I don't know if this is part of that scope. More thought is required here.

Some of the implementations are perfect, however. The ones that have a one-to-one mapping from union clause ast to instruction:

How does one determine which are one-to-one mappings? The generally accurate and easy way is to look at the associated mapping clause assembly, on the right side of the <->. If there is a single mnemonic string followed immediately by "spc()", that is a one-to-one.

Possibly obvious, but I'll note that there are a great many more instructions in other riscv_insts_*.sail that need the same treatment.

If this is fine I will also try to implement Alasdair Armstrong's approach for #3

3 would be a nice one, but it's very big and I think difficult. You have been warned. ;-) I would welcome the attempt!

jriyyya commented 9 months ago

Possibly obvious, but I'll note that there are a great many more instructions in other riscv_insts_*.sail that need the same treatment.

I am looking for some feasible and better solution for doing so

3 would be a nice one, but it's very big and I think difficult. You have been warned. ;-) I would welcome the attempt!

Yes, I realized that now, Though it's worth a try :p

jriyyya commented 7 months ago

@ThinkOpenly Hello, I checked your suggested changes and have applied them. Also for the one where you said the line has to be deleted, I just wanted to ensure if just the line has to be deleted or the whole description of it was also to be deleted.

jriyyya commented 7 months ago

Thanks for considering this PR! I think we can merge this now.