diff mbox

[C++] Remove last use of can_trust_pointer_alignment

Message ID alpine.LNX.2.00.1108101206180.810@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Aug. 10, 2011, 10:26 a.m. UTC 
Nothing in the middle-end happens conditional on 
can_trust_pointer_alignment anymore - we can always "trust" pointer
alignment, that function and its comment is somewhat gross.
In fact we can now track alignment properly via CCP and thus
the hack in cfgexpand.c:expand_call_stmt should not be neccessary
anymore.

Now, looking at the use of can_trust_pointer_alignment in build_over_call
and especially its comment:

          if (!can_trust_pointer_alignment ())
            {
              /* If we can't be sure about pointer alignment, a call
                 to __builtin_memcpy is expanded as a call to memcpy, which
                 is invalid with identical args.  Otherwise it is
                 expanded as a block move, which should be safe.  */
              arg0 = save_expr (arg0);
              arg1 = save_expr (arg1);
              test = build2 (EQ_EXPR, boolean_type_node, arg0, arg1);
            }

"Otherwise it is expanded as a block move" is certainly not true.
Whether it is expanded as block move depends on optimization setting,
target costs and, well, a way to do block moves (we fall back to
memcpy after all).

So the patch changes the above to if (!optimize), but I'd argue
we can as well either remove the conditional or emit it unconditional.
Dependent on whether we believe a memcpy implementation exists in
the wild that asserts that src != dst.

Independent of the can_trust_pointer_alignment issue nowadays the
case of !tree_int_cst_equal (TYPE_SIZE (type), TYPE_SIZE (as_base))
should use a MODIFY_EXPR with two MEM_REF operands and a proper
alias set, which would avoid the address-taking (and thus aliasing)
because of the memcpy and also would avoid forcing alias-set zero
because of the memcpy.

Thus (untested), something like

Comments

Richard Biener Aug. 10, 2011, 12:35 p.m. UTC | #1 
On Wed, 10 Aug 2011, Richard Guenther wrote:

> 
> Nothing in the middle-end happens conditional on 
> can_trust_pointer_alignment anymore - we can always "trust" pointer
> alignment, that function and its comment is somewhat gross.
> In fact we can now track alignment properly via CCP and thus
> the hack in cfgexpand.c:expand_call_stmt should not be neccessary
> anymore.
> 
> Now, looking at the use of can_trust_pointer_alignment in build_over_call
> and especially its comment:
> 
>           if (!can_trust_pointer_alignment ())
>             {
>               /* If we can't be sure about pointer alignment, a call
>                  to __builtin_memcpy is expanded as a call to memcpy, which
>                  is invalid with identical args.  Otherwise it is
>                  expanded as a block move, which should be safe.  */
>               arg0 = save_expr (arg0);
>               arg1 = save_expr (arg1);
>               test = build2 (EQ_EXPR, boolean_type_node, arg0, arg1);
>             }
> 
> "Otherwise it is expanded as a block move" is certainly not true.
> Whether it is expanded as block move depends on optimization setting,
> target costs and, well, a way to do block moves (we fall back to
> memcpy after all).
> 
> So the patch changes the above to if (!optimize), but I'd argue
> we can as well either remove the conditional or emit it unconditional.
> Dependent on whether we believe a memcpy implementation exists in
> the wild that asserts that src != dst.

The following patch changes us to unconditionally call memcpy, like
the backend may decide to do for the
tree_int_cst_equal (TYPE_SIZE (type), TYPE_SIZE (as_base))
MODIFY_EXPR case.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

Ok?

Thanks,
Richard.

2011-08-10  Richard Guenther  <rguenther@suse.de>

	* tree.h (can_trust_pointer_alignment): Remove.
	* builtins.c (can_trust_pointer_alignment): Remove.

	cp/
	* call.c (build_over_call): Call memcpy unconditionally.

Index: trunk/gcc/builtins.c
===================================================================
*** trunk.orig/gcc/builtins.c	2011-08-10 14:25:04.000000000 +0200
--- trunk/gcc/builtins.c	2011-08-10 14:25:53.000000000 +0200
*************** get_object_alignment (tree exp)
*** 453,468 ****
    return align;
  }
  
- /* Returns true iff we can trust that alignment information has been
-    calculated properly.  */
- 
- bool
- can_trust_pointer_alignment (void)
- {
-   /* We rely on TER to compute accurate alignment information.  */
-   return (optimize && flag_tree_ter);
- }
- 
  /* Return the alignment in bits of EXP, a pointer valued expression.
     The alignment returned is, by default, the alignment of the thing that
     EXP points to.  If it is not a POINTER_TYPE, 0 is returned.
--- 453,458 ----
Index: trunk/gcc/cp/call.c
===================================================================
*** trunk.orig/gcc/cp/call.c	2011-08-10 14:19:23.000000000 +0200
--- trunk/gcc/cp/call.c	2011-08-10 14:26:58.000000000 +0200
*************** build_over_call (struct z_candidate *can
*** 6767,6799 ****
        else
  	{
  	  /* We must only copy the non-tail padding parts.
! 	     Use __builtin_memcpy for the bitwise copy.
! 	     FIXME fix 22488 so we can go back to using MODIFY_EXPR
! 	     instead of an explicit call to memcpy.  */
! 	
  	  tree arg0, arg1, arg2, t;
- 	  tree test = NULL_TREE;
  
  	  arg2 = TYPE_SIZE_UNIT (as_base);
  	  arg1 = arg;
  	  arg0 = cp_build_addr_expr (to, complain);
  
- 	  if (!can_trust_pointer_alignment ())
- 	    {
- 	      /* If we can't be sure about pointer alignment, a call
- 		 to __builtin_memcpy is expanded as a call to memcpy, which
- 		 is invalid with identical args.  Otherwise it is
- 		 expanded as a block move, which should be safe.  */
- 	      arg0 = save_expr (arg0);
- 	      arg1 = save_expr (arg1);
- 	      test = build2 (EQ_EXPR, boolean_type_node, arg0, arg1);
- 	    }
  	  t = implicit_built_in_decls[BUILT_IN_MEMCPY];
  	  t = build_call_n (t, 3, arg0, arg1, arg2);
  
  	  t = convert (TREE_TYPE (arg0), t);
- 	  if (test)
- 	    t = build3 (COND_EXPR, TREE_TYPE (t), test, arg0, t);
  	  val = cp_build_indirect_ref (t, RO_NULL, complain);
            TREE_NO_WARNING (val) = 1;
  	}
--- 6767,6783 ----
        else
  	{
  	  /* We must only copy the non-tail padding parts.
! 	     Use __builtin_memcpy for the bitwise copy.  */
  	  tree arg0, arg1, arg2, t;
  
  	  arg2 = TYPE_SIZE_UNIT (as_base);
  	  arg1 = arg;
  	  arg0 = cp_build_addr_expr (to, complain);
  
  	  t = implicit_built_in_decls[BUILT_IN_MEMCPY];
  	  t = build_call_n (t, 3, arg0, arg1, arg2);
  
  	  t = convert (TREE_TYPE (arg0), t);
  	  val = cp_build_indirect_ref (t, RO_NULL, complain);
            TREE_NO_WARNING (val) = 1;
  	}
Index: trunk/gcc/tree.h
===================================================================
*** trunk.orig/gcc/tree.h	2011-08-10 14:25:04.000000000 +0200
--- trunk/gcc/tree.h	2011-08-10 14:25:37.000000000 +0200
*************** extern tree build_va_arg_indirect_ref (t
*** 5358,5364 ****
  extern tree build_string_literal (int, const char *);
  extern bool validate_arglist (const_tree, ...);
  extern rtx builtin_memset_read_str (void *, HOST_WIDE_INT, enum machine_mode);
- extern bool can_trust_pointer_alignment (void);
  extern bool is_builtin_name (const char *);
  extern bool is_builtin_fn (tree);
  extern unsigned int get_object_alignment_1 (tree, unsigned HOST_WIDE_INT *);
--- 5358,5363 ----
Jason Merrill Aug. 10, 2011, 2:54 p.m. UTC | #2 
On 08/10/2011 08:35 AM, Richard Guenther wrote:
> 	* call.c (build_over_call): Call memcpy unconditionally.

OK.  Have you tested the MEM_REF patch?

Jason
Richard Biener Aug. 10, 2011, 3:01 p.m. UTC | #3 
On Wed, 10 Aug 2011, Jason Merrill wrote:

> On 08/10/2011 08:35 AM, Richard Guenther wrote:
> > 	* call.c (build_over_call): Call memcpy unconditionally.
> 
> OK.  Have you tested the MEM_REF patch?

No, not yet.  I'll throw it to testing now.

Richard.
James Greenhalgh Aug. 12, 2011, 9:15 a.m. UTC | #4 
> 2011-08-10  Richard Guenther  <rguenther@suse.de>
> 
> 	* tree.h (can_trust_pointer_alignment): Remove.
> 	* builtins.c (can_trust_pointer_alignment): Remove.
> 
> 	cp/
> 	* call.c (build_over_call): Call memcpy unconditionally.
> 

Hi,

This appears to have caused a regression on arm-unknown-eabi

*** EXIT code 4242
FAIL: g++.dg/init/copy7.C execution test

Thanks,
James Greenhalgh
Richard Biener Aug. 12, 2011, 9:34 a.m. UTC | #5 
On Fri, 12 Aug 2011, James Greenhalgh wrote:

> > 2011-08-10  Richard Guenther  <rguenther@suse.de>
> > 
> > 	* tree.h (can_trust_pointer_alignment): Remove.
> > 	* builtins.c (can_trust_pointer_alignment): Remove.
> > 
> > 	cp/
> > 	* call.c (build_over_call): Call memcpy unconditionally.
> > 
> 
> Hi,
> 
> This appears to have caused a regression on arm-unknown-eabi
> 
> *** EXIT code 4242
> FAIL: g++.dg/init/copy7.C execution test

Ah, that looks "expected".  Can you verify the regression persists
at r177691?  If so it is a generic middle-end issue (see also
comment #18 mentioned in the PR).

Thanks,
Richard.
diff mbox

Patch

Index: gcc/cp/call.c
===================================================================
--- gcc/cp/call.c       (revision 177597)
+++ gcc/cp/call.c       (working copy)
@@ -6379,35 +6379,21 @@  build_over_call (struct z_candidate *can
        }
       else
        {
-         /* We must only copy the non-tail padding parts.
-            Use __builtin_memcpy for the bitwise copy.
-            FIXME fix 22488 so we can go back to using MODIFY_EXPR
-            instead of an explicit call to memcpy.  */
-
-         tree arg0, arg1, arg2, t;
+         /* We must only copy the non-tail padding parts.  */
+         tree arg0, arg2, t;
          tree test = NULL_TREE;
 
          arg2 = TYPE_SIZE_UNIT (as_base);
-         arg1 = arg;
          arg0 = cp_build_addr_expr (to, complain);
 
-         if (!can_trust_pointer_alignment ())
-           {
-             /* If we can't be sure about pointer alignment, a call
-                to __builtin_memcpy is expanded as a call to memcpy, 
which
-                is invalid with identical args.  Otherwise it is
-                expanded as a block move, which should be safe.  */
-             arg0 = save_expr (arg0);
-             arg1 = save_expr (arg1);
-             test = build2 (EQ_EXPR, boolean_type_node, arg0, arg1);
-           }
-         t = implicit_built_in_decls[BUILT_IN_MEMCPY];
-         t = build_call_n (t, 3, arg0, arg1, arg2);
-
-         t = convert (TREE_TYPE (arg0), t);
-         if (test)
-           t = build3 (COND_EXPR, TREE_TYPE (t), test, arg0, t);
-         val = cp_build_indirect_ref (t, RO_NULL, complain);
+         t = build_array_type (char_type_node, build_index_type (arg2));
+         t = build2 (MODIFY_EXPR, void_type_node,
+                     build2 (MEM_REF, t, to,
+                             build_int_cst (build_pointer_type (type), 
0)),
+                     build2 (MEM_REF, t, arg,
+                             build_int_cst (build_pointer_type (type), 
0)));
+         val = build2 (COMPOUND_EXPR, TREE_TYPE (to),
+                       t, to);
           TREE_NO_WARNING (val) = 1;
        }
 
which does a block copy of size arg2 (by using a char[arg2] array
a type for the mem-ref) using the alias-set of type (by
specifying the offset of the mem-ref with using a type of type *).

Well, but I'm bootstrapping and testing the following now, on
x86_64-unknown-linux-gnu (because I want to get rid of
that can_trust_pointer_alignment function).

Ok for trunk?

Thanks,
Richard.

2011-08-10  Richard Guenther  <rguenther@suse.de>

	cp/
	* call.c (build_over_call): Check for optimize instead of
	can_trust_pointer_alignment.  Adjust comment.

Index: gcc/cp/call.c
===================================================================
--- gcc/cp/call.c	(revision 177613)
+++ gcc/cp/call.c	(working copy)
@@ -6778,12 +6778,14 @@  build_over_call (struct z_candidate *can
 	  arg1 = arg;
 	  arg0 = cp_build_addr_expr (to, complain);
 
-	  if (!can_trust_pointer_alignment ())
+	  if (!optimize)
 	    {
-	      /* If we can't be sure about pointer alignment, a call
-		 to __builtin_memcpy is expanded as a call to memcpy, which
-		 is invalid with identical args.  Otherwise it is
-		 expanded as a block move, which should be safe.  */
+	      /* If we are not optimizing, a call to __builtin_memcpy is
+		 expanded as a call to memcpy, which is invalid with identical
+		 args.  Otherwise it is expanded as a block move, which should
+		 be safe.  */
+	      /* ???  It is of course only sometimes expanded as a block
+		 move.  */
 	      arg0 = save_expr (arg0);
 	      arg1 = save_expr (arg1);
 	      test = build2 (EQ_EXPR, boolean_type_node, arg0, arg1);