> -----Original Message-----
> From: Brian Cain <bc...@quicinc.com>
> Sent: Thursday, September 1, 2022 4:30 PM
> To: qemu-devel@nongnu.org; Taylor Simpson <tsimp...@quicinc.com>
> Cc: Richard Henderson <richard.hender...@linaro.org>; Brian Cain
> <bc...@quicinc.com>
> Subject: [PATCH] Hexagon (target/hexagon) implement mutability mask for
> GPRs
>
> Some registers are defined to have immutable bits, this commit will
> implement that behavior.
>
> Signed-off-by: Brian Cain <bc...@quicinc.com>
> ---
> target/hexagon/gen_masked.c | 44 ++++++++++++
> target/hexagon/gen_masked.h | 26 ++++++++
> target/hexagon/genptr.c | 33 ++++++++-
> target/hexagon/hex_regs.h | 6 ++
> target/hexagon/meson.build | 1 +
> tests/tcg/hexagon/Makefile.target | 1 +
> tests/tcg/hexagon/reg_mut.c | 107
> ++++++++++++++++++++++++++++++
> 7 files changed, 217 insertions(+), 1 deletion(-) create mode 100644
> target/hexagon/gen_masked.c create mode 100644
> target/hexagon/gen_masked.h create mode 100644
> tests/tcg/hexagon/reg_mut.c
>
> diff --git a/target/hexagon/gen_masked.c b/target/hexagon/gen_masked.c
> new file mode 100644 index 0000000000..faffee2e13
Run
git config diff.orderFile scripts/git.orderFile
in your workspace.
This will ensure (among other things) that the .h files appear in the patch
before the .c files. This makes it easier for the reviewers.
> --- /dev/null
> +++ b/target/hexagon/gen_masked.c
> @@ -0,0 +1,44 @@
> +void gen_masked_reg_write(TCGv cur_val, TCGv in_val, TCGv out_val,
> + target_ulong reg_mask) {
> + TCGv set_bits = tcg_temp_new();
> + TCGv cleared_bits = tcg_temp_new();
> +
> + /*
> + * set_bits = in_val & reg_mask
> + * cleared_bits = (~in_val) & reg_mask
> + */
> + tcg_gen_andi_tl(set_bits, in_val, reg_mask);
> + tcg_gen_not_tl(cleared_bits, in_val);
> + tcg_gen_andi_tl(cleared_bits, cleared_bits, reg_mask);
> +
> + /*
> + * result = (reg_cur | set_bits) & (~cleared_bits)
> + */
> + tcg_gen_not_tl(cleared_bits, cleared_bits);
> + tcg_gen_or_tl(set_bits, set_bits, cur_val);
> + tcg_gen_and_tl(out_val, set_bits, cleared_bits);
> +
> + tcg_temp_free(set_bits);
> + tcg_temp_free(cleared_bits);
> +}
In addition to Richard's feedback, you should do nothing when reg_mask is zero.
> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> 8a334ba07b..21385f556e 100644
> --- a/target/hexagon/genptr.c
> +++ b/target/hexagon/genptr.c
> static inline void gen_log_reg_write(int rnum, TCGv val) {
> - tcg_gen_mov_tl(hex_new_value[rnum], val);
> + const hexagon_mut_entry entry = gpr_mut_masks[rnum];
> + if (entry.present) {
> + gen_masked_reg_write(hex_gpr[rnum], val, hex_new_value[rnum],
You can't write to hex_gpr here. You have to wait to make sure the packet will
commit. Put this result back into val and do the mov to hex_new_value
unconditionally.
> + entry.mask);
> + } else {
> + tcg_gen_mov_tl(hex_new_value[rnum], val);
> + }
> static void gen_log_predicated_reg_write_pair(int rnum, TCGv_i64 val, int
> slot) {
> + const hexagon_mut_entry entry0 = gpr_mut_masks[rnum];
> + const hexagon_mut_entry entry1 = gpr_mut_masks[rnum + 1];
> TCGv val32 = tcg_temp_new();
> TCGv zero = tcg_constant_tl(0);
> TCGv slot_mask = tcg_temp_new();
> + TCGv tmp_val = tcg_temp_new();
>
> tcg_gen_andi_tl(slot_mask, hex_slot_cancelled, 1 << slot);
> +
> /* Low word */
> tcg_gen_extrl_i64_i32(val32, val);
> + if (entry0.present) {
> + tcg_gen_mov_tl(tmp_val, val32);
> + gen_masked_reg_write(hex_gpr[rnum], tmp_val, val32, entry0.mask);
See previous comment. Put the result back into val32 and let the subsequent
code put it into hex_new_value if the slot isn't cancelled.
> + tcg_temp_free(tmp_val);
> + }
> tcg_gen_movcond_tl(TCG_COND_EQ, hex_new_value[rnum],
> slot_mask, zero,
> val32, hex_new_value[rnum]);
> +
> /* High word */
> tcg_gen_extrh_i64_i32(val32, val);
> + if (entry1.present) {
> + tcg_gen_mov_tl(tmp_val, val32);
> + gen_masked_reg_write(hex_gpr[rnum], tmp_val, val32, entry1.mask);
Ditto.
> + }
> a/target/hexagon/hex_regs.h b/target/hexagon/hex_regs.h index
> a63c2c0fd5..db48cff96e 100644
> --- a/target/hexagon/hex_regs.h
> +++ b/target/hexagon/hex_regs.h
> @@ -79,6 +79,12 @@ enum {
> HEX_REG_QEMU_HVX_CNT = 54,
> HEX_REG_UTIMERLO = 62,
> HEX_REG_UTIMERHI = 63,
> + HEX_REG_LAST_VALUE = 64,
You can use TOTAL_PER_THREAD_REGS (defined in cpu.h).
> };
> diff --git a/tests/tcg/hexagon/reg_mut.c b/tests/tcg/hexagon/reg_mut.c
> new file mode 100644 index 0000000000..7e81ec6bf3
> --- /dev/null
> +++ b/tests/tcg/hexagon/reg_mut.c
> @@ -0,0 +1,107 @@
> +#define READ_REG(reg_name, out_reg) \
> + asm volatile ("%0 = " reg_name "\n\t" \
> + : "=r"(out_reg) \
> + : \
> + : \
> + ); \
> +
Remove this - it isn't used.
> +int main()
> +{
> + check(set_usr(0x00), 0x00);
> + check(set_usr(0xffffffff), 0x3ecfff3f);
> + check(set_usr(0x00), 0x00);
> + check(set_usr(0x01), 0x01);
> + check(set_usr(0xff), 0x3f);
> +
> + /*
> + * PC is special. Setting it to these values
> + * should cause an instruction fetch miss.
Why would there be an instruction fetch miss? The gpr_mut_masks[HEX_REG_PC] is
zero, so this instruction won't change PC.
> + */
> + check_ne(set_pc(0x00000000), 0x00000000);
> + check_ne(set_pc(0xffffffff), 0xffffffff);
> + check_ne(set_pc(0x00000001), 0x00000001);
> + check_ne(set_pc(0x000000ff), 0x000000ff);
> +
> + puts(err ? "FAIL" : "PASS");
> + return err;
> +}
Add some tests that do the assignment inside a packet and using a register pair
assignment.
Thanks,
Taylor
