Re: [RFC, PR 80689] Copy small aggregates element-wise

[Richard Biener <richard.guenther@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2850 bytes --]

On Fri, Nov 24, 2017 at 11:31 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Nov 23, 2017 at 4:32 PM, Martin Jambor <mjambor@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.

Like the attached.  At least FAILs

FAIL: gcc.dg/tree-ssa/ssa-ccp-27.c scan-tree-dump-times ccp1
"memcpy[^\n]*123456" 2 (found 0 times)

not sure why we have this test.

Richard.

[-- Attachment #2: p3-2 --]
[-- Type: application/octet-stream, Size: 7170 bytes --]

Index: gcc/gimple-fold.c
===================================================================
--- gcc/gimple-fold.c	(revision 255137)
+++ gcc/gimple-fold.c	(working copy)
@@ -925,161 +925,38 @@ gimple_fold_builtin_memory_op (gimple_st
 	  return false;
 	}
 
-      if (!tree_fits_shwi_p (len))
+      if (!tree_fits_uhwi_p (len))
 	return false;
-      /* FIXME:
-         This logic lose for arguments like (type *)malloc (sizeof (type)),
-         since we strip the casts of up to VOID return value from malloc.
-	 Perhaps we ought to inherit type from non-VOID argument here?  */
-      STRIP_NOPS (src);
-      STRIP_NOPS (dest);
+
       if (!POINTER_TYPE_P (TREE_TYPE (src))
 	  || !POINTER_TYPE_P (TREE_TYPE (dest)))
 	return false;
-      /* In the following try to find a type that is most natural to be
-	 used for the memcpy source and destination and that allows
-	 the most optimization when memcpy is turned into a plain assignment
-	 using that type.  In theory we could always use a char[len] type
-	 but that only gains us that the destination and source possibly
-	 no longer will have their address taken.  */
-      /* As we fold (void *)(p + CST) to (void *)p + CST undo this here.  */
-      if (TREE_CODE (src) == POINTER_PLUS_EXPR)
-	{
-	  tree tem = TREE_OPERAND (src, 0);
-	  STRIP_NOPS (tem);
-	  if (tem != TREE_OPERAND (src, 0))
-	    src = build1 (NOP_EXPR, TREE_TYPE (tem), src);
-	}
-      if (TREE_CODE (dest) == POINTER_PLUS_EXPR)
-	{
-	  tree tem = TREE_OPERAND (dest, 0);
-	  STRIP_NOPS (tem);
-	  if (tem != TREE_OPERAND (dest, 0))
-	    dest = build1 (NOP_EXPR, TREE_TYPE (tem), dest);
-	}
-      srctype = TREE_TYPE (TREE_TYPE (src));
-      if (TREE_CODE (srctype) == ARRAY_TYPE
-	  && !tree_int_cst_equal (TYPE_SIZE_UNIT (srctype), len))
-	{
-	  srctype = TREE_TYPE (srctype);
-	  STRIP_NOPS (src);
-	  src = build1 (NOP_EXPR, build_pointer_type (srctype), src);
-	}
-      desttype = TREE_TYPE (TREE_TYPE (dest));
-      if (TREE_CODE (desttype) == ARRAY_TYPE
-	  && !tree_int_cst_equal (TYPE_SIZE_UNIT (desttype), len))
-	{
-	  desttype = TREE_TYPE (desttype);
-	  STRIP_NOPS (dest);
-	  dest = build1 (NOP_EXPR, build_pointer_type (desttype), dest);
-	}
-      if (TREE_ADDRESSABLE (srctype)
-	  || TREE_ADDRESSABLE (desttype))
-	return false;
 
-      /* Make sure we are not copying using a floating-point mode or
-         a type whose size possibly does not match its precision.  */
-      if (FLOAT_MODE_P (TYPE_MODE (desttype))
-	  || TREE_CODE (desttype) == BOOLEAN_TYPE
-	  || TREE_CODE (desttype) == ENUMERAL_TYPE)
-	desttype = bitwise_type_for_mode (TYPE_MODE (desttype));
-      if (FLOAT_MODE_P (TYPE_MODE (srctype))
-	  || TREE_CODE (srctype) == BOOLEAN_TYPE
-	  || TREE_CODE (srctype) == ENUMERAL_TYPE)
-	srctype = bitwise_type_for_mode (TYPE_MODE (srctype));
-      if (!srctype)
-	srctype = desttype;
-      if (!desttype)
-	desttype = srctype;
-      if (!srctype)
+      /* We want the possibility of a decl becoming non-addressable.  */
+      if (! ((TREE_CODE (dest) == ADDR_EXPR
+	      && var_decl_component_p (TREE_OPERAND (dest, 0)))
+	     || (TREE_CODE (src) == ADDR_EXPR
+		 && var_decl_component_p (TREE_OPERAND (src, 0)))))
 	return false;
 
+      /* Use an unsigned char[] type to perform the copying to preserve
+         padding and to avoid any issues with TREE_ADDRESSABLE types
+	 or float modes behavior on copying.  */
+      desttype = build_array_type_nelts (unsigned_char_type_node,
+					 tree_to_uhwi (len));
+      srctype = desttype;
+
+      /* Preserve higher pointer alignment.  */
       src_align = get_pointer_alignment (src);
       dest_align = get_pointer_alignment (dest);
-      if (dest_align < TYPE_ALIGN (desttype)
-	  || src_align < TYPE_ALIGN (srctype))
-	return false;
-
-      destvar = dest;
-      STRIP_NOPS (destvar);
-      if (TREE_CODE (destvar) == ADDR_EXPR
-	  && var_decl_component_p (TREE_OPERAND (destvar, 0))
-	  && tree_int_cst_equal (TYPE_SIZE_UNIT (desttype), len))
-	destvar = fold_build2 (MEM_REF, desttype, destvar, off0);
-      else
-	destvar = NULL_TREE;
-
-      srcvar = src;
-      STRIP_NOPS (srcvar);
-      if (TREE_CODE (srcvar) == ADDR_EXPR
-	  && var_decl_component_p (TREE_OPERAND (srcvar, 0))
-	  && tree_int_cst_equal (TYPE_SIZE_UNIT (srctype), len))
-	{
-	  if (!destvar
-	      || src_align >= TYPE_ALIGN (desttype))
-	    srcvar = fold_build2 (MEM_REF, destvar ? desttype : srctype,
-				  srcvar, off0);
-	  else if (!STRICT_ALIGNMENT)
-	    {
-	      srctype = build_aligned_type (TYPE_MAIN_VARIANT (desttype),
-					    src_align);
-	      srcvar = fold_build2 (MEM_REF, srctype, srcvar, off0);
-	    }
-	  else
-	    srcvar = NULL_TREE;
-	}
-      else
-	srcvar = NULL_TREE;
-
-      if (srcvar == NULL_TREE && destvar == NULL_TREE)
-	return false;
-
-      if (srcvar == NULL_TREE)
-	{
-	  STRIP_NOPS (src);
-	  if (src_align >= TYPE_ALIGN (desttype))
-	    srcvar = fold_build2 (MEM_REF, desttype, src, off0);
-	  else
-	    {
-	      if (STRICT_ALIGNMENT)
-		return false;
-	      srctype = build_aligned_type (TYPE_MAIN_VARIANT (desttype),
-					    src_align);
-	      srcvar = fold_build2 (MEM_REF, srctype, src, off0);
-	    }
-	}
-      else if (destvar == NULL_TREE)
-	{
-	  STRIP_NOPS (dest);
-	  if (dest_align >= TYPE_ALIGN (srctype))
-	    destvar = fold_build2 (MEM_REF, srctype, dest, off0);
-	  else
-	    {
-	      if (STRICT_ALIGNMENT)
-		return false;
-	      desttype = build_aligned_type (TYPE_MAIN_VARIANT (srctype),
-					     dest_align);
-	      destvar = fold_build2 (MEM_REF, desttype, dest, off0);
-	    }
-	}
+      if (src_align > TYPE_ALIGN (srctype))
+	srctype = build_aligned_type (srctype, src_align);
+      if (dest_align > TYPE_ALIGN (desttype))
+	desttype = build_aligned_type (desttype, dest_align);
 
-      gimple *new_stmt;
-      if (is_gimple_reg_type (TREE_TYPE (srcvar)))
-	{
-	  tree tem = fold_const_aggregate_ref (srcvar);
-	  if (tem)
-	    srcvar = tem;
-	  if (! is_gimple_min_invariant (srcvar))
-	    {
-	      new_stmt = gimple_build_assign (NULL_TREE, srcvar);
-	      srcvar = create_tmp_reg_or_ssa_name (TREE_TYPE (srcvar),
-						   new_stmt);
-	      gimple_assign_set_lhs (new_stmt, srcvar);
-	      gimple_set_vuse (new_stmt, gimple_vuse (stmt));
-	      gsi_insert_before (gsi, new_stmt, GSI_SAME_STMT);
-	    }
-	}
-      new_stmt = gimple_build_assign (destvar, srcvar);
+      gimple *new_stmt
+	= gimple_build_assign (fold_build2 (MEM_REF, desttype, dest, off0),
+			       fold_build2 (MEM_REF, srctype, src, off0));
       gimple_set_vuse (new_stmt, gimple_vuse (stmt));
       gimple_set_vdef (new_stmt, gimple_vdef (stmt));
       if (gimple_vdef (new_stmt)
@@ -6121,7 +5998,10 @@ gimple_fold_stmt_to_constant_1 (gimple *
 		tree op0 = (*valueize) (gimple_assign_rhs1 (stmt));
 		tree op1 = (*valueize) (gimple_assign_rhs2 (stmt));
 		if (TREE_CODE (op0) == ADDR_EXPR
-		    && TREE_CODE (op1) == INTEGER_CST)
+		    && TREE_CODE (op1) == INTEGER_CST
+		    && (!cfun
+			|| cfun->after_inlining
+			|| !handled_component_p (TREE_OPERAND (op0, 0))))
 		  {
 		    tree off = fold_convert (ptr_type_node, op1);
 		    return build_fold_addr_expr_loc