Re: [PATCH] Hexagon (target/hexagon) implement mutability mask for GPRs

[Richard Henderson]

On 9/1/22 22:29, Brian Cain wrote:
> +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);

This is overly complicated.  It should be

    out = (in & mask) | (cur & ~mask)

which is only 3 operations instead of 6:

    tcg_gen_andi_tl(t1, in_val, reg_mask);
    tcg_gen_andi_tl(t2, cur_val, ~reg_mask);
    tcg_gen_or_tl(out_val, t1, t2);


I'm surprised about new files for this simple operation.  Surely a subroutine within 
genptr.c would be sufficient.


> +static const hexagon_mut_entry gpr_mut_masks[HEX_REG_LAST_VALUE] = {
> +    [HEX_REG_PC] = {true, 0x00000000},
> +    [HEX_REG_GP] = {true, 0xffffffc0},
> +    [HEX_REG_USR] = {true, 0x3ecfff3f},
> +    [HEX_REG_UTIMERLO] = {true, 0x00000000},
> +    [HEX_REG_UTIMERHI] = {true, 0x00000000},
> +};
...
>   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],
> +            entry.mask);
> +    } else {
> +        tcg_gen_mov_tl(hex_new_value[rnum], val);
> +    }

You could avoid the structure and .present flag by initializing all other entries to 
UINT32_MAX.  E.g.

static const target_ulong gpr_mut_masks[HEX_REG_LAST_VALUE] = {
    [0 ... HEX_REG_LAST_VALUE - 1] = UINT32_MAX,
    [HEX_REG_PC] = 0
    ...
};

It might be clearer, and easier to initialize, if you invert the sense of the mask: only 
set bits that are immutable, so that you get

static const target_ulong gpr_immutable_masks[HEX_REG_LAST_VALUE] = {
    [HEX_REG_PC] = UINT32_MAX,
    [HEX_REG_GP] = 0x3f,
    ...
};


r~