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 <[email protected]>
* 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
