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? 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. 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