On Thu, Nov 23, 2017 at 4:32 PM, Martin Jambor <mjam...@suse.cz> wrote:
> Hi,
>
> On Mon, Nov 13 2017, Richard Biener wrote:
>> The main concern here is that GIMPLE is not very well defined for
>> aggregate copies and that gimple-fold.c happily optimizes
>> memcpy (&a, &b, sizeof (a)) into a = b;
>>
>> struct A { short s; long i; long j; };
>> struct A a, b;
>> void foo ()
>> {
>> __builtin_memcpy (&a, &b, sizeof (struct A));
>> }
>>
>> gets folded to
>>
>> MEM[(char * {ref-all})&a] = MEM[(char * {ref-all})&b];
>> return;
>>
>> you see we're careful about TBAA but (don't see that above but
>> can be verified by for example debugging expand_assignment)
>> TREE_TYPE (MEM[...]) is actually 'struct A'.
>>
>> And yes, I've been worried about SRA as well here... it _does_
>> have some early outs when seeing VIEW_CONVERT_EXPR but
>> appearantly not for the above. Testcase that aborts with SRA but
>> not without:
>>
>> struct A { short s; long i; long j; };
>> struct A a, b;
>> void foo ()
>> {
>> struct A c;
>> __builtin_memcpy (&c, &b, sizeof (struct A));
>> __builtin_memcpy (&a, &c, sizeof (struct A));
>> }
>> int main()
>> {
>> __builtin_memset (&b, 0, sizeof (struct A));
>> b.s = 1;
>> __builtin_memcpy ((char *)&b+2, &b, 2);
>> foo ();
>> __builtin_memcpy (&a, (char *)&a+2, 2);
>> if (a.s != 1)
>> __builtin_abort ();
>> return 0;
>> }
>
> Thanks for the testcase, I agree that is a fairly big problem. Do you
> think that the following (untested) patch is an appropriate way of
> fixing it and generally of extending gimple to capture that a statement
> is a bit-copy?
I think the place to fix is the memcpy folding. That is, we'd say that
aggregate assignments are not bit-copies but do element-wise assignments.
For memcpy folding we'd then need to use a type that doesn't contain
padding. Which effectively means char[].
Of course we need to stop SRA from decomposing that copy to
individual characters then ;)
So iff we decide that all aggregate copies are element copies,
maybe only those where TYPE_MAIN_VARIANT of lhs and rhs match
(currently we allow TYPE_CANONICAL compatibility and thus there
might be some mismatches), then we have to fix nothign but
the memcpy folding.
> If so, I'll add the testcase, bootstrap it and formally propose it.
> Subsequently I will of course make sure that any element-wise copying
> patch would test the predicate.
I don't think the alias-set should determine whether a copy is
bit-wise or not.
Richard.
> Thanks,
>
> Martin
>
>
> 2017-11-23 Martin Jambor <mjam...@suse.cz>
>
> * gimple.c (gimple_bit_copy_p): New function.
> * gimple.h (gimple_bit_copy_p): Declare it.
> * tree-sra.c (sra_modify_assign): Use it.
> ---
> gcc/gimple.c | 20 ++++++++++++++++++++
> gcc/gimple.h | 1 +
> gcc/tree-sra.c | 1 +
> 3 files changed, 22 insertions(+)
>
> diff --git a/gcc/gimple.c b/gcc/gimple.c
> index c986a732004..e1b428d91bb 100644
> --- a/gcc/gimple.c
> +++ b/gcc/gimple.c
> @@ -3087,6 +3087,26 @@ gimple_inexpensive_call_p (gcall *stmt)
> return false;
> }
>
> +/* Return true if STMT is an assignment performing bit copy and so is also
> + expected to copy any padding. */
> +
> +bool
> +gimple_bit_copy_p (gassign *stmt)
> +{
> + if (!gimple_assign_single_p (stmt))
> + return false;
> +
> + tree lhs = gimple_assign_lhs (stmt);
> + if (TREE_CODE (lhs) == MEM_REF
> + && TYPE_REF_CAN_ALIAS_ALL (reference_alias_ptr_type (lhs)))
> + return true;
> + tree rhs = gimple_assign_rhs1 (stmt);
> + if (TREE_CODE (rhs) == MEM_REF
> + && TYPE_REF_CAN_ALIAS_ALL (reference_alias_ptr_type (rhs)))
> + return true;
> + return false;
> +}
> +
> #if CHECKING_P
>
> namespace selftest {
> diff --git a/gcc/gimple.h b/gcc/gimple.h
> index 334def89398..60929473361 100644
> --- a/gcc/gimple.h
> +++ b/gcc/gimple.h
> @@ -1531,6 +1531,7 @@ extern void gimple_seq_discard (gimple_seq);
> extern void maybe_remove_unused_call_args (struct function *, gimple *);
> extern bool gimple_inexpensive_call_p (gcall *);
> extern bool stmt_can_terminate_bb_p (gimple *);
> +extern bool gimple_bit_copy_p (gassign *);
>
> /* Formal (expression) temporary table handling: multiple occurrences of
> the same scalar expression are evaluated into the same temporary. */
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index db490b20c3e..fc0a8fe60bf 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -3591,6 +3591,7 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator
> *gsi)
> || gimple_has_volatile_ops (stmt)
> || contains_vce_or_bfcref_p (rhs)
> || contains_vce_or_bfcref_p (lhs)
> + || gimple_bit_copy_p (as_a <gassign *> (stmt))
> || stmt_ends_bb_p (stmt))
> {
> /* No need to copy into a constant-pool, it comes pre-initialized. */
> --
> 2.15.0
>
