RE: [PATCH][GCC][AARCH64]Bad code-gen for structure/block/unaligned memory access

[Tamar Christina <Tamar dot Christina at arm dot com>]

> > +  /* Optimize routines for MEM to REG copies.  */  if (n < 8 &&
> > + !REG_P (XEXP (operands[0], 0)))
> 
> This seems to be checking that the address of the original destination
> memory isn't a plain base register.  Why's it important to reject that case but
> allow e.g. base+offset?
> 

In the case of e.g.

void Fun3(struct struct3 foo3)
{
  L3 = foo3;
}


> > +   {
> > +     unsigned int max_align = UINTVAL (operands[2]);
> > +     max_align = n < max_align ? max_align : n;
> 
> Might be misunderstanding, but isn't max_align always equal to n here, since
> n was set by:
> 
>   n = UINTVAL (operands[2]);
> 
> Indentation of the enclosing { ... } is slightly off.
> 
> > +     machine_mode mov_mode, dest_mode
> > +       = smallest_mode_for_size (max_align * BITS_PER_UNIT, MODE_INT);
> > +     rtx result = gen_reg_rtx (dest_mode);
> > +     emit_insn (gen_move_insn (result, GEN_INT (0)));
> > +
> > +     unsigned int shift_cnt = 0;
> > +     for (; n > shift_cnt; shift_cnt += GET_MODE_SIZE (mov_mode))
> > +       {
> > +	 int nearest = 0;
> > +	 /* Find the mode to use, but limit the max to TI mode.  */
> > +	 for (unsigned max = 1; max <= (n - shift_cnt) && max <= 16; max *=
> 2)
> > +	      nearest = max;
> 
> In the if statement above, you required n < 8, so can max ever by > 16 here?
> 
> > +	  mov_mode = smallest_mode_for_size (nearest * BITS_PER_UNIT,
> MODE_INT);
> > +	  rtx reg = gen_reg_rtx (mov_mode);
> > +
> > +	  src = adjust_address (src, mov_mode, 0);
> > +	  emit_insn (gen_move_insn (reg, src));
> > +	  src = aarch64_progress_pointer (src);
> > +
> > +	  reg = gen_rtx_ASHIFT (dest_mode, reg,
> > +				GEN_INT (shift_cnt * BITS_PER_UNIT));
> 
> This seems to be mixing modes: reg has mode "mov_mode" but the result
> has mode "dest_mode".  That isn't well-formed: the mode of a shift result
> needs to be the same as the mode of the operand.  I think the load would
> need to be a zero-extend of "src" into "reg", with "reg" having mode
> "dest_mode".
> 
> > +	  result = gen_rtx_IOR (dest_mode, reg, result);
> > +      }
> > +
> > +    dst = adjust_address (dst, dest_mode, 0);
> > +    emit_insn (gen_move_insn (dst, result));
> 
> dest_mode was chosen by smallest_mode_for_size, so can be bigger than n.
> Doesn't that mean that we'll write beyond the end of the copy region when
> n is an awkward number?
> 
> > diff --git a/gcc/expr.c b/gcc/expr.c
> > index
> >
> 91d7ea217229fac62380b5d4b646961bf7c836c1..b1df4651e7942346007cda1cce
> 8e
> > e5a19297ab16 100644
> > --- a/gcc/expr.c
> > +++ b/gcc/expr.c
> > @@ -2743,7 +2743,9 @@ copy_blkmode_to_reg (machine_mode mode,
> tree
> > src)
> >
> >    n_regs = (bytes + UNITS_PER_WORD - 1) / UNITS_PER_WORD;
> >    dst_words = XALLOCAVEC (rtx, n_regs);
> > -  bitsize = MIN (TYPE_ALIGN (TREE_TYPE (src)), BITS_PER_WORD);
> > +  bitsize = BITS_PER_WORD;
> > +  if (SLOW_UNALIGNED_ACCESS (BLKmode, TYPE_ALIGN (TREE_TYPE
> (src))))
> > +    bitsize = MIN (TYPE_ALIGN (TREE_TYPE (src)), BITS_PER_WORD);
> 
> I think this ought to be testing word_mode instead of BLKmode.
> (Testing BLKmode doesn't really make sense in general, because the mode
> doesn't have a meaningful alignment.)
> 
> Thanks,
> Richard