> -----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


Reply via email to