This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
RE: [PATCH][GCC][AARCH64]Bad code-gen for structure/block/unaligned memory access
- From: Tamar Christina <Tamar dot Christina at arm dot com>
- To: Richard Sandiford <richard dot sandiford at linaro dot org>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, nd <nd at arm dot com>, James Greenhalgh <James dot Greenhalgh at arm dot com>, Richard Earnshaw <Richard dot Earnshaw at arm dot com>, Marcus Shawcroft <Marcus dot Shawcroft at arm dot com>
- Date: Fri, 9 Jun 2017 15:51:52 +0000
- Subject: RE: [PATCH][GCC][AARCH64]Bad code-gen for structure/block/unaligned memory access
- Authentication-results: sourceware.org; auth=none
- Authentication-results: linaro.org; dkim=none (message not signed) header.d=none;linaro.org; dmarc=none action=none header.from=arm.com;
- Nodisclaimer: True
- References: <VI1PR0801MB20313B74A43EAC4756E8C8BDFFC80@VI1PR0801MB2031.eurprd08.prod.outlook.com> <87a85ic3i1.fsf@linaro.org>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
> > + /* 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?
this function is (as far as I could tell) only being called with two types of destinations:
a location on the stack or a plain register.
When the destination is a register such as with
void Fun3(struct struct3 foo3)
{
L3 = foo3;
}
You run into the issue you had pointed to below where we might write too much. Ideally the constraint
I would like is to check if the destination is either a new register (no data) or that the structure was padded.
I couldn't figure out how to do this and the gains over the existing code for this case was quite small.
So I just disallowed it leaving it to the existing code, which isn't so bad, only 1 extra instruction.
>
> > + {
> > + 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:
Correct, previously this patch was made to allow n < 16. These were left over from the cleanup.
I'll correct.
>
> > + 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?
Yes, see my answer above. For the other case, when we write onto a location on the stack, this is fine
due to the alignment.
>
> > 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.)
Ah, yes that makes sense. I'll update the patch.
New patch is validating and will submit it soon.
>
> Thanks,
> Richard