Skip to content

Commit

Permalink
Fix Store-Conditional assembly operand order and add parens
Browse files Browse the repository at this point in the history
The operand order for Store-Conditional assembly has the second
and third operands reversed.

The RISC-V Instruction Set Manual states:
> ```
> SC.W conditionally writes a word in rs2 to the address in rs1 [...]
> If the SC.W succeeds, the instruction writes the word in rs2 to memory,
> and it writes zero to rd. If the SC.W fails, the instruction does not
> write to memory, and it writes a nonzero value to rd.
> ```

`rd` is for the return code, `rs2` is the value, and `rs1` is the memory
address.

For the syntax `sc.w A,B,(C)`:
- `A` is where the result is stored, per convention. So, this is `rd`.
- `B` is the value to be stored. So, this is `rs2`.
- `C` is the address at which to store the value. So, this is `rs1`.

The resulting syntax would be `stc.w rd,rs2,(rs1)`.

The current assembly representation is:
```
"sc." ^ size_mnemonic(size) [...] reg_name(rd) ^ sep() ^ reg_name(rs1) ^ sep() ^ reg_name(rs2)
```

Note that the order is wrong. In addition, parentheses are missing around `rs2`.

Fix this instance, as well as two other instances where parentheses are missing.

Fixes riscv#338.
Fixes riscv#344.

Suggested-by: Tim Hutt <[email protected]>
  • Loading branch information
ThinkOpenly authored and billmcspadden-riscv committed Nov 29, 2023
1 parent 928dd86 commit b414bae
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 4 deletions.
4 changes: 2 additions & 2 deletions model/riscv_insts_aext.sail
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ function clause execute(LOADRES(aq, rl, rs1, width, rd)) = {
}

mapping clause assembly = LOADRES(aq, rl, rs1, size, rd)
<-> "lr." ^ size_mnemonic(size) ^ maybe_aq(aq) ^ maybe_rl(rl) ^ spc() ^ reg_name(rd) ^ sep() ^ reg_name(rs1)
<-> "lr." ^ size_mnemonic(size) ^ maybe_aq(aq) ^ maybe_rl(rl) ^ spc() ^ reg_name(rd) ^ sep() ^ "(" ^ reg_name(rs1) ^ ")"

/* ****************************************************************** */
union clause ast = STORECON : (bool, bool, regidx, regidx, word_width, regidx)
Expand Down Expand Up @@ -251,7 +251,7 @@ function clause execute (STORECON(aq, rl, rs2, rs1, width, rd)) = {
}

mapping clause assembly = STORECON(aq, rl, rs2, rs1, size, rd)
<-> "sc." ^ size_mnemonic(size) ^ maybe_aq(aq) ^ maybe_rl(rl) ^ spc() ^ reg_name(rd) ^ sep() ^ reg_name(rs1) ^ sep() ^ reg_name(rs2)
<-> "sc." ^ size_mnemonic(size) ^ maybe_aq(aq) ^ maybe_rl(rl) ^ spc() ^ reg_name(rd) ^ sep() ^ reg_name(rs2) ^ sep() ^ "(" ^ reg_name(rs1) ^ ")"

/* ****************************************************************** */
union clause ast = AMO : (amoop, bool, bool, regidx, regidx, word_width, regidx)
Expand Down
4 changes: 2 additions & 2 deletions model/riscv_insts_base.sail
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ mapping clause encdec = RISCV_JALR(imm, rs1, rd)
<-> imm @ rs1 @ 0b000 @ rd @ 0b1100111

mapping clause assembly = RISCV_JALR(imm, rs1, rd)
<-> "jalr" ^ spc() ^ reg_name(rd) ^ sep() ^ reg_name(rs1) ^ sep() ^ hex_bits_12(imm)
<-> "jalr" ^ spc() ^ reg_name(rd) ^ sep() ^ hex_bits_12(imm) ^ "(" ^ reg_name(rs1) ^ ")"

/* see riscv_jalr_seq.sail or riscv_jalr_rmem.sail for the execute clause. */

Expand Down Expand Up @@ -430,7 +430,7 @@ mapping maybe_u = {
}

mapping clause assembly = LOAD(imm, rs1, rd, is_unsigned, size, aq, rl)
<-> "l" ^ size_mnemonic(size) ^ maybe_u(is_unsigned) ^ maybe_aq(aq) ^ maybe_rl(rl) ^ spc() ^ reg_name(rd) ^ sep() ^ hex_bits_12(imm) ^ opt_spc() ^ "(" ^ opt_spc() ^ reg_name(rs1) ^ opt_spc() ^ ")"
<-> "l" ^ size_mnemonic(size) ^ maybe_u(is_unsigned) ^ maybe_aq(aq) ^ maybe_rl(rl) ^ spc() ^ reg_name(rd) ^ sep() ^ hex_bits_12(imm) ^ "(" ^ reg_name(rs1) ^ ")"

/* ****************************************************************** */
union clause ast = STORE : (bits(12), regidx, regidx, word_width, bool, bool)
Expand Down

0 comments on commit b414bae

Please sign in to comment.