ThinkOpenly / sail-riscv

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

Add pseudoinstruction VNEG #33

Open Linda-Njau opened 2 months ago

Linda-Njau commented 2 months ago

This change adds the pseudoinstruction VNEG that maps to VX_VRSUB, following the steps outlined in issue #6.

From the RISC-V Instruction Set Manual Volume I, VNEG is described as:

vneg.v vd,vs = vrsub.vx vd,vs,x0

After running make json the output for this pseudoinstruction is:

{
  "mnemonic": "vneg.v",
  "name": "TBD",
  "operands": [ { "name": "vd", "type": "regidx", "optional": false },{ "name": "vs", "type": "regidx", "optional": false } ],
  "syntax": "vd,vs",
  "format": "TBD",
  "fields": [ { "field": "encdec(VXTYPE(VX_VRSUB,0b0,vs,reg_name(x0),vd))", "size": 0 } ],
  "extensions": [ "V" ],
  "function": "execute(VXTYPE(VX_VRSUB, 0b0, vs, reg_name(\"x0\"), vd))",
  "description": "TBD"
}
Linda-Njau commented 2 months ago

I've noticed the fields attribute in the JSON output isn't right. I'll work on it tomorrow and try to fix it before you review it. If I get stuck, I'll ask for help.

Linda-Njau commented 2 months ago

Update on the fields attribute of the JSON output. I added the zext.w pseudoinstruction as defined in the comment on issue #6. Its JSON output for the fields attribute is similar to the output of the pseudoinstruction vneg.v that I previously added. So this leads me to think that we may need to adjust how pseudoinstructions are extracted into the JSON.

 "mnemonic": "zext.w",
  "name": "TBD",
  "operands": [ { "name": "rd", "type": "regidx", "optional": false },{ "name": "rs1", "type": "regidx", "optional": false } ],
  "syntax": "rd,rs1",
  "format": "TBD",
  "fields": [ { "field": "encdec(ZBA_RTYPEUW(reg_name(zero),rs1,rd,RISCV_ADDUW))", "size": 0 } ],
  "extensions": [ "Zba" ],
  "function": "execute(ZBA_RTYPEUW(reg_name(\"zero\"), rs1, rd, RISCV_ADDUW))",
  "description": "TBD"
ThinkOpenly commented 2 months ago
{
  "mnemonic": "vneg.v",
  "name": "TBD",
  "operands": [ { "name": "vd", "type": "regidx", "optional": false },{ "name": "vs", "type": "regidx", "optional": false } ],
  "syntax": "vd,vs",
  "format": "TBD",
  "fields": [ { "field": "encdec(VXTYPE(VX_VRSUB,0b0,vs,reg_name(x0),vd))", "size": 0 } ],
  "extensions": [ "V" ],
  "function": "execute(VXTYPE(VX_VRSUB, 0b0, vs, reg_name(\"x0\"), vd))",
  "description": "TBD"
}

Yes, this is pretty much what is expected at the moment.

Where we need to take these is to add some new handling, as you surmised:

ThinkOpenly commented 2 months ago

Interesting. The CI checks failed in one of the test cases, rv32mi-p-shamt.elf. It appears that they run the tests via test/run_tests.sh. Want to investigate that?

Linda-Njau commented 2 months ago

Interesting. The CI checks failed in one of the test cases, rv32mi-p-shamt.elf. It appears that they run the tests via test/run_tests.sh. Want to investigate that?

Yes, I'm looking into it.

Linda-Njau commented 2 months ago

Where we need to take these is to add some new handling, as you surmised:

  • The content above is fine, and can be left as-is
  • Since we now have the pseudo_of mapping, we can utilize that to emit new information in the JSON, something like a new top-level section:

I see. We can discuss more on the implementation and its priority level

ThinkOpenly commented 1 month ago

@Linda-Njau , I see some "Add names" commits are appearing in this branch. Could you move them to a separate branch, so this branch covers only the one topic, "pseudoinstructions", please?

Linda-Njau commented 1 month ago

@Linda-Njau , I see some "Add names" commits are appearing in this branch. Could you move them to a separate branch, so this branch covers only the one topic, "pseudoinstructions", please?

Done : )