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

[Max Filippov]

Hi Richard,

On Tue, May 18, 2021 at 1:11 PM Richard Henderson
 wrote:
> On 5/17/21 3:52 PM, Max Filippov wrote:
> > @@ -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.:

That indeed looks better. Let me make another version of this patch.
Thanks for taking a look!

-- 
Thanks.
-- Max

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~