Re: [PATCH v3] target/xtensa: clean up unaligned access

[Richard Henderson]

On 5/17/21 3:52 PM, Max Filippov wrote:
> -static void gen_load_store_alignment(DisasContext *dc, int shift,
> -        TCGv_i32 addr, bool no_hw_alignment)
> +static MemOp gen_load_store_alignment(DisasContext *dc, int shift,
> +                                      TCGv_i32 addr, bool no_hw_alignment)
>   {
>       if (!option_enabled(dc, XTENSA_OPTION_UNALIGNED_EXCEPTION)) {
>           tcg_gen_andi_i32(addr, addr, ~0 << shift);
> -    } else if (option_enabled(dc, XTENSA_OPTION_HW_ALIGNMENT) &&
> -            no_hw_alignment) {
> -        TCGLabel *label = gen_new_label();
> -        TCGv_i32 tmp = tcg_temp_new_i32();
> -        tcg_gen_andi_i32(tmp, addr, ~(~0 << shift));
> -        tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 0, label);
> -        gen_exception_cause_vaddr(dc, LOAD_STORE_ALIGNMENT_CAUSE, addr);
> -        gen_set_label(label);
> -        tcg_temp_free(tmp);
> +    }
> +    if (!no_hw_alignment && option_enabled(dc, XTENSA_OPTION_HW_ALIGNMENT)) {
> +        return MO_UNALN;
> +    } else {
> +        return MO_ALIGN;
>       }
>   }
>   
> @@ -1784,10 +1770,11 @@ static void translate_l32e(DisasContext *dc, const OpcodeArg arg[],
>                              const uint32_t par[])
>   {
>       TCGv_i32 addr = tcg_temp_new_i32();
> +    MemOp al;
>   
>       tcg_gen_addi_i32(addr, arg[1].in, arg[2].imm);
> -    gen_load_store_alignment(dc, 2, addr, false);
> -    tcg_gen_qemu_ld_tl(arg[0].out, addr, dc->ring, MO_TEUL);
> +    al = gen_load_store_alignment(dc, 2, addr, false);
> +    tcg_gen_qemu_ld_tl(arg[0].out, addr, dc->ring, MO_TEUL | al);

You're duplicating the information about the size of the alignment.

I think it would be better to pass the partial MemOp into 
get_load_store_alignment and return the complete MemOp.  E.g.:

MemOp gen_load_store_alignment(DisasContext *dc, MemOp mop,
                               TCGv addr)
{
    if ((mop & MO_SIZE) == MO_8) {
        return mop;
    }
    if ((mop & MO_AMASK) == 0 &&
        !option_enabled(dc, XTENSA_OPTION_HW_ALIGNMENT)) {
        mop |= MO_ALIGN;
    }

    if (!option_enabled(dc, XTENSA_OPTION_UNALIGNED_EXCEPTION)) {
        tcg_gen_andi_i32(addr, addr,
                         ~0 << get_alignment_bits(mop));
    }
    return mop;
}

Then used as

    mop = gen_load_store_alignment(dc, MO_TEUL, addr);
    tcg_gen_qemu_ld_tl(arg[0].out, addr, dc->ring, mop);

    mop = gen_load_store_alignment(dc, MO_TEUL | MO_ALIGN, addr);
    gen_check_exclusive(dc, addr, false);
    tcg_gen_qemu_ld_i32(arg[0].out, addr, dc->ring, mop);

This organization does require that you remove TARGET_ALIGNED_ONLY=y from 
default-configs/targets/xtensa*.mak so that MO_ALIGN has the non-zero value.


r~