This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix PR tree-optimization/51315
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: Eric Botcazou <ebotcazou at adacore dot com>
- Cc: Richard Guenther <richard dot guenther at gmail dot com>, gcc-patches at gcc dot gnu dot org
- Date: Tue, 03 Jan 2012 11:36:51 +0000
- Subject: Re: [PATCH] Fix PR tree-optimization/51315
- References: <201112062324.09783.ebotcazou@adacore.com> <201112071532.59754.ebotcazou@adacore.com> <87fwfyorox.fsf@firetop.home> <201201031206.05214.ebotcazou@adacore.com>
Eric Botcazou <ebotcazou@adacore.com> writes:
>> Sorry for the late notice, but this regressed memcpy-1.c for n32 and n64
>> on mips64-linux-gnu. I realise memcpy-1.c isn't viewed as being a proper
>> SRA test, but in this case I think it really is showing a genuine problem.
>> We have:
>>
>> struct a {int a,b,c;} a;
>> int test(struct a a)
>> {
>> struct a nasty_local;
>> __builtin_memcpy (&nasty_local,&a, sizeof(a));
>> return nasty_local.a;
>> }
>>
>> We apply LOCAL_ALIGNMENT to nasty_local during estimated_stack_frame_size,
>> so we have a VAR_DECL (nasty_local) with 64-bit alignment and a PARM_DECL
>> (a) with 32-bit alignment. This fails the condition:
>>
>> if (STRICT_ALIGNMENT
>> && tree_non_aligned_mem_p (rhs, get_object_alignment (lhs)))
>> lacc->grp_unscalarizable_region = 1;
>>
>> because LHS has 64-bit alignment but RHS has 32-bit alignment.
>
> Do you mean that the patch pessimizes this case?
Yeah.
> If so, yes, this is known, I ran into similar cases in Ada (we do this
> kind of local alignment promotion).
OK. But passing small structures by value doesn't seem that rare --
especially in C++ -- and it doesn't feel right to disable SRA just because
the backend likes to increase the alignment of stack vars. So...
> Index: tree-sra.c
> ===================================================================
> --- tree-sra.c (revision 182780)
> +++ tree-sra.c (working copy)
> @@ -1124,7 +1124,9 @@ build_accesses_from_assign (gimple stmt)
> {
> lacc->grp_assignment_write = 1;
> if (STRICT_ALIGNMENT
> - && tree_non_aligned_mem_p (rhs, get_object_alignment (lhs)))
> + && tree_non_aligned_mem_p (rhs,
> + MIN (TYPE_ALIGN (lacc->type),
> + get_object_alignment (lhs))))
> lacc->grp_unscalarizable_region = 1;
> }
>
> @@ -1135,7 +1137,9 @@ build_accesses_from_assign (gimple stmt)
> && !is_gimple_reg_type (racc->type))
> bitmap_set_bit (should_scalarize_away_bitmap, DECL_UID (racc->base));
> if (STRICT_ALIGNMENT
> - && tree_non_aligned_mem_p (lhs, get_object_alignment (rhs)))
> + && tree_non_aligned_mem_p (lhs,
> + MIN (TYPE_ALIGN (racc->type),
> + get_object_alignment (rhs))))
> racc->grp_unscalarizable_region = 1;
> }
>
> on the grounds that sub-accesses shouldn't be more aligned. I think this
> should be OK but, well...
...something like this sounds good, although you seem less than happy
with it :-)
(I suppose we shouldn't literally use MIN on get_object_alignment,
since it's a bit too expensive to evaluate twice.)
Richard