Convert (type *)&A into &A->field_of_type_and_offset_0.

Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

Hi,
this patch teach fold_unary to convert (type *)&A into
&A->field_of_type_and_offset_0. These constructs are common in C++ inheritance
and the strange cast is a stopper for various further optimizations.
The transformation matches couple hounderd times compiling Gerald's testcase,
but also on GCC bootstrap (in parser, genattrtab and other places)

Bootstrapped/regtested i686-linux, OK?
:ADDPATCH tree-optimization:
Honza
	* fold-const.c (find_subfield_of_type): New function.
	(fold_unary): Convert (type *)&A into &A->field_of_type_and_offset_0.
Index: fold-const.c
===================================================================
--- fold-const.c	(revision 123515)
+++ fold-const.c	(working copy)
@@ -7497,6 +7497,44 @@ fold_view_convert_expr (tree type, tree 
   return native_interpret_expr (type, buffer, len);
 }
 
+/* Look recursively into subtypes of ITYPE and see if there is field of OTYPE
+   at offset 0.  DEPTH is depth of recurssion.
+   If one is found, return malloc allocated array containing sequence of
+   FIELD_DECLs that are nestying into subtime up to the sought after field. 
+   
+   This is used to convert (type *)&A into &A->field_of_type_and_offset_0.  */
+static tree *
+find_subfield_of_type (tree otype, tree itype, int depth)
+{
+  if (TREE_CODE (itype) == RECORD_TYPE
+      || TREE_CODE (itype) == UNION_TYPE
+      || TREE_CODE (itype) == QUAL_UNION_TYPE)
+    {
+      tree field;
+
+      for (field = TYPE_FIELDS (itype); field; field = TREE_CHAIN (field))
+	if (TREE_CODE (field) == FIELD_DECL
+	    && integer_zerop (byte_position (field)))
+	  {
+	    tree *ret;
+
+	    if (TREE_TYPE (field) == otype)
+	      {
+	        tree *ret = xmalloc ((depth + 2) * sizeof (tree *));
+		ret[depth+1] = NULL;
+		ret[depth] = field;
+		return ret;
+	      }
+	    else if ((ret = find_subfield_of_type (otype, TREE_TYPE (field),
+		     depth + 1)) != NULL)
+	      {
+	        ret[depth] = field;
+		return ret;
+	      }
+	  }
+    }
+  return NULL;
+}
+
 
 /* Fold a unary expression of code CODE and type TYPE with operand
    OP0.  Return the folded expression if folding is successful.
@@ -7508,6 +7546,7 @@ fold_unary (enum tree_code code, tree ty
   tree tem;
   tree arg0;
   enum tree_code_class kind = TREE_CODE_CLASS (code);
+  tree *fields;
 
   gcc_assert (IS_EXPR_CODE_CLASS (kind)
 	      && TREE_CODE_LENGTH (code) == 1);
@@ -7735,6 +7774,42 @@ fold_unary (enum tree_code code, tree ty
 	    return fold_convert (type, build_fold_addr_expr (base));
         }
 
+      /* Convert (type *)&A into &A->field_of_type_and_offset_0.  */
+      if (TREE_CODE (op0) == ADDR_EXPR && TREE_CODE (type) == POINTER_TYPE
+	  && (fields = find_subfield_of_type (TREE_TYPE (type),
+					      TREE_TYPE (TREE_OPERAND (op0, 0)), 0)))
+	{
+	  tree ret = TREE_OPERAND (op0, 0);
+	  int i;
+	  for (i = 0; fields[i]; i++)
+	    ret = build3 (COMPONENT_REF, TREE_TYPE (fields[i]), ret, fields[i], NULL);
+	  free (fields);
+	  return build_fold_addr_expr_with_type (ret, type);
+	}
+
       if ((TREE_CODE (op0) == MODIFY_EXPR
 	   || TREE_CODE (op0) == GIMPLE_MODIFY_STMT)
 	  && TREE_CONSTANT (GENERIC_TREE_OPERAND (op0, 1))

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Richard Guenther]

On 4/6/07, Jan Hubicka <jh@suse.cz> wrote:
> Hi,
> this patch teach fold_unary to convert (type *)&A into
> &A->field_of_type_and_offset_0. These constructs are common in C++ inheritance
> and the strange cast is a stopper for various further optimizations.
> The transformation matches couple hounderd times compiling Gerald's testcase,
> but also on GCC bootstrap (in parser, genattrtab and other places)
> Bootstrapped/regtested i686-linux, OK?

I think tree-ssa-ccp.c:maybe_fold_offset_to_component_ref has
some "interesting" side-cases and does not require memory allocation
but passes back component_refs, that looks better in my view.

Richard.

> :ADDPATCH tree-optimization:
> Honza
>         * fold-const.c (find_subfield_of_type): New function.
>         (fold_unary): Convert (type *)&A into &A->field_of_type_and_offset_0.

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

> On 4/6/07, Jan Hubicka <jh@suse.cz> wrote:
> >Hi,
> >this patch teach fold_unary to convert (type *)&A into
> >&A->field_of_type_and_offset_0. These constructs are common in C++ 
> >inheritance
> >and the strange cast is a stopper for various further optimizations.
> >The transformation matches couple hounderd times compiling Gerald's 
> >testcase,
> >but also on GCC bootstrap (in parser, genattrtab and other places)
> >Bootstrapped/regtested i686-linux, OK?
> 
> I think tree-ssa-ccp.c:maybe_fold_offset_to_component_ref has
> some "interesting" side-cases and does not require memory allocation
> but passes back component_refs, that looks better in my view.

Hmm, maybe_fold_offset_to_component_ref should've probably matched the
case I was looking into, but it didn't because it tries to recurse only
on the first field overlapping with offset specified, so on union and
offset of 0, it will goto found at first field of sufficient size and
won't look into rest of union. At least if I am reading the code right.

I however think the packing of COMPONENT_REFs there is quite poor: if we
don't have match, we can produce quite few dead COMPONENT_REFs since we
tend to fold each statement many times and if we produce a testcase with
many "bad guys" producing deep nest that don't result in transformation,
we would end up consuming a lot of memory. My implementation was done in
that way to do malloc/free pair IFF the transformation suceeds, that
should be cheaper overall.

I would probably duplicate "found" code into the loop so the loop can be
continued when match is not found on first union member. 

To avoid allocation, we can either have the list of refs like I do in my
code that might be dificult because various refs are needed, or we can
simply search twice - once without building the containers and if match
is found, second time doing the job...

Honza
> 
> Richard.

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

> > On 4/6/07, Jan Hubicka <jh@suse.cz> wrote:
> > >Hi,
> > >this patch teach fold_unary to convert (type *)&A into
> > >&A->field_of_type_and_offset_0. These constructs are common in C++ 
> > >inheritance
> > >and the strange cast is a stopper for various further optimizations.
> > >The transformation matches couple hounderd times compiling Gerald's 
> > >testcase,
> > >but also on GCC bootstrap (in parser, genattrtab and other places)
> > >Bootstrapped/regtested i686-linux, OK?
> > 
> > I think tree-ssa-ccp.c:maybe_fold_offset_to_component_ref has
> > some "interesting" side-cases and does not require memory allocation
> > but passes back component_refs, that looks better in my view.
> 
> Hmm, maybe_fold_offset_to_component_ref should've probably matched the
> case I was looking into, but it didn't because it tries to recurse only
> on the first field overlapping with offset specified, so on union and
> offset of 0, it will goto found at first field of sufficient size and
> won't look into rest of union. At least if I am reading the code right.

Hi,
actually the reason seems to be as simple as the fact that the function
is never called, since it is considered only to fold away INDIRECT_REFs
of something.  The folding is useful for bare ADDR_EXPRs too, so this
patch adds the call.

I've also added calls with nonzero offset, since the functionally is
readilly available. It would be nice if we can fold away this way a
multiple inheritance, that does not work (just in artifical testcase I
get match).  This is because we would need to combine the two gimple
statements, that probably forward propagation can be told about. I
guess it is best to proceed incrementally. (Ie I would still like to try
to avoid unnecesary allocations and get the function working on unions
where fild field is not the match)

I am testing the patch on x86_64-linux, does it seem to make more sense
now?

	* tree.h (maybe_fold_offset_to_component_ref): Declare.
	* fold-const.c (fold_unary, fold_binary): Fold NOPs of ADDR_EXPR
	into COMPONENT_REFs.
	* tree-ssa-ccp.c (maybe_fold_offset_to_component_ref): Export.
Index: tree.h
===================================================================
--- tree.h	(revision 123515)
+++ tree.h	(working copy)
@@ -4412,6 +4412,8 @@ extern void fold_defer_overflow_warnings
 extern void fold_undefer_overflow_warnings (bool, tree, int);
 extern void fold_undefer_and_ignore_overflow_warnings (void);
 extern bool fold_deferring_overflow_warnings_p (void);
+extern tree maybe_fold_offset_to_component_ref (tree, tree, tree,
+				                tree, bool);
 
 extern tree force_fit_type_double (tree, unsigned HOST_WIDE_INT, HOST_WIDE_INT,
 				   int, bool);
Index: fold-const.c
===================================================================
--- fold-const.c	(revision 123515)
+++ fold-const.c	(working copy)
@@ -7735,6 +7775,23 @@ fold_unary (enum tree_code code, tree ty
 	    return fold_convert (type, build_fold_addr_expr (base));
         }
 
+      /* Convert (type *)&A into &A->field_of_type_and_offset_0.  */
+      if (TREE_CODE (op0) == ADDR_EXPR && POINTER_TYPE_P (type)
+	  && (tem = maybe_fold_offset_to_component_ref
+		      (TREE_TYPE (TREE_OPERAND (op0, 0)), TREE_OPERAND (op0, 0),
+		       integer_zero_node, TREE_TYPE (type), false)))
+        return build_fold_addr_expr_with_type (tem, type);
+
+      /* Convert (type *)&(A+offset) into &A->field_of_type_and_offset.  */
+      if (TREE_CODE (op0) == PLUS_EXPR
+	  && TREE_CODE (TREE_OPERAND (op0, 0)) == ADDR_EXPR
+	  && TREE_CODE (TREE_OPERAND (op0, 1)) == INTEGER_CST
+	  && (tem = maybe_fold_offset_to_component_ref
+		      (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (op0, 0), 0)),
+		       TREE_OPERAND (TREE_OPERAND (op0, 0), 0),
+		       TREE_OPERAND (op0, 1), TREE_TYPE (type), false)))
+        return build_fold_addr_expr_with_type (tem, type);
+
       if ((TREE_CODE (op0) == MODIFY_EXPR
 	   || TREE_CODE (op0) == GIMPLE_MODIFY_STMT)
 	  && TREE_CONSTANT (GENERIC_TREE_OPERAND (op0, 1))
@@ -9227,6 +9284,15 @@ fold_binary (enum tree_code code, tree t
 	      if (tem)
 		return fold_convert (type, tem);
 	    }
+          /* Convert ((type *)&A)+offset into &A->field_of_type_and_offset.  */
+	  if (TREE_CODE (op0) == NOP_EXPR
+	      && TREE_CODE (TREE_OPERAND (op0, 0)) == ADDR_EXPR
+	      && TREE_CODE (op1) == INTEGER_CST
+	      && (tem = maybe_fold_offset_to_component_ref
+			  (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (op0, 0), 0)),
+			   TREE_OPERAND (TREE_OPERAND (op0, 0), 0),
+			   op1, TREE_TYPE (type), false)))
+	    return build_fold_addr_expr_with_type (tem, type);
 	}
       else
 	{
Index: tree-ssa-ccp.c
===================================================================
--- tree-ssa-ccp.c	(revision 123515)
+++ tree-ssa-ccp.c	(working copy)
@@ -1643,7 +1643,7 @@ maybe_fold_offset_to_array_ref (tree bas
    is the desired result type.  */
 /* ??? This doesn't handle class inheritance.  */
 
-static tree
+tree
 maybe_fold_offset_to_component_ref (tree record_type, tree base, tree offset,
 				    tree orig_type, bool base_is_ptr)
 {

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Andrew Pinski]

On 4/6/07, Jan Hubicka <hubicka@ucw.cz> wrote:
> I am testing the patch on x86_64-linux, does it seem to make more sense
> now?

Maybe add a testcase or two?

Thanks,
Andrew Pinski

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Richard Guenther]

On 4/7/07, Jan Hubicka <hubicka@ucw.cz> wrote:
> > > On 4/6/07, Jan Hubicka <jh@suse.cz> wrote:
> > > >Hi,
> > > >this patch teach fold_unary to convert (type *)&A into
> > > >&A->field_of_type_and_offset_0. These constructs are common in C++
> > > >inheritance
> > > >and the strange cast is a stopper for various further optimizations.
> > > >The transformation matches couple hounderd times compiling Gerald's
> > > >testcase,
> > > >but also on GCC bootstrap (in parser, genattrtab and other places)
> > > >Bootstrapped/regtested i686-linux, OK?
> > >
> > > I think tree-ssa-ccp.c:maybe_fold_offset_to_component_ref has
> > > some "interesting" side-cases and does not require memory allocation
> > > but passes back component_refs, that looks better in my view.
> >
> > Hmm, maybe_fold_offset_to_component_ref should've probably matched the
> > case I was looking into, but it didn't because it tries to recurse only
> > on the first field overlapping with offset specified, so on union and
> > offset of 0, it will goto found at first field of sufficient size and
> > won't look into rest of union. At least if I am reading the code right.
>
> Hi,
> actually the reason seems to be as simple as the fact that the function
> is never called, since it is considered only to fold away INDIRECT_REFs
> of something.  The folding is useful for bare ADDR_EXPRs too, so this
> patch adds the call.
>
> I've also added calls with nonzero offset, since the functionally is
> readilly available. It would be nice if we can fold away this way a
> multiple inheritance, that does not work (just in artifical testcase I
> get match).  This is because we would need to combine the two gimple
> statements, that probably forward propagation can be told about. I
> guess it is best to proceed incrementally. (Ie I would still like to try
> to avoid unnecesary allocations and get the function working on unions
> where fild field is not the match)
>
> I am testing the patch on x86_64-linux, does it seem to make more sense
> now?

Yes.  As Andrew mentions, a few testcases would be nice.  And...

>         * tree.h (maybe_fold_offset_to_component_ref): Declare.
>         * fold-const.c (fold_unary, fold_binary): Fold NOPs of ADDR_EXPR
>         into COMPONENT_REFs.
>         * tree-ssa-ccp.c (maybe_fold_offset_to_component_ref): Export.
> Index: tree.h
> ===================================================================
> --- tree.h      (revision 123515)
> +++ tree.h      (working copy)
> @@ -4412,6 +4412,8 @@ extern void fold_defer_overflow_warnings
>  extern void fold_undefer_overflow_warnings (bool, tree, int);
>  extern void fold_undefer_and_ignore_overflow_warnings (void);
>  extern bool fold_deferring_overflow_warnings_p (void);
> +extern tree maybe_fold_offset_to_component_ref (tree, tree, tree,
> +                                               tree, bool);
>
>  extern tree force_fit_type_double (tree, unsigned HOST_WIDE_INT, HOST_WIDE_INT,
>                                    int, bool);
> Index: fold-const.c
> ===================================================================
> --- fold-const.c        (revision 123515)
> +++ fold-const.c        (working copy)
> @@ -7735,6 +7775,23 @@ fold_unary (enum tree_code code, tree ty
>             return fold_convert (type, build_fold_addr_expr (base));
>          }
>
> +      /* Convert (type *)&A into &A->field_of_type_and_offset_0.  */
> +      if (TREE_CODE (op0) == ADDR_EXPR && POINTER_TYPE_P (type)
> +         && (tem = maybe_fold_offset_to_component_ref
> +                     (TREE_TYPE (TREE_OPERAND (op0, 0)), TREE_OPERAND (op0, 0),
> +                      integer_zero_node, TREE_TYPE (type), false)))
> +        return build_fold_addr_expr_with_type (tem, type);
> +
> +      /* Convert (type *)&(A+offset) into &A->field_of_type_and_offset.  */
> +      if (TREE_CODE (op0) == PLUS_EXPR
> +         && TREE_CODE (TREE_OPERAND (op0, 0)) == ADDR_EXPR
> +         && TREE_CODE (TREE_OPERAND (op0, 1)) == INTEGER_CST
> +         && (tem = maybe_fold_offset_to_component_ref
> +                     (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (op0, 0), 0)),
> +                      TREE_OPERAND (TREE_OPERAND (op0, 0), 0),
> +                      TREE_OPERAND (op0, 1), TREE_TYPE (type), false)))
> +        return build_fold_addr_expr_with_type (tem, type);

especially for this one - does this really happen?  I guess the comment should
read (type *)(&A + offset) and here I would expect we fold it right to the case
below, (type *)&A + offset'.

>        if ((TREE_CODE (op0) == MODIFY_EXPR
>            || TREE_CODE (op0) == GIMPLE_MODIFY_STMT)
>           && TREE_CONSTANT (GENERIC_TREE_OPERAND (op0, 1))
> @@ -9227,6 +9284,15 @@ fold_binary (enum tree_code code, tree t
>               if (tem)
>                 return fold_convert (type, tem);
>             }
> +          /* Convert ((type *)&A)+offset into &A->field_of_type_and_offset.  */
> +         if (TREE_CODE (op0) == NOP_EXPR
> +             && TREE_CODE (TREE_OPERAND (op0, 0)) == ADDR_EXPR
> +             && TREE_CODE (op1) == INTEGER_CST
> +             && (tem = maybe_fold_offset_to_component_ref
> +                         (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (op0, 0), 0)),
> +                          TREE_OPERAND (TREE_OPERAND (op0, 0), 0),
> +                          op1, TREE_TYPE (type), false)))
> +           return build_fold_addr_expr_with_type (tem, type);

Do we handle &A + offset (without the cast) somewhere already in fold?

Richard.

>         }
>        else
>         {
> Index: tree-ssa-ccp.c
> ===================================================================
> --- tree-ssa-ccp.c      (revision 123515)
> +++ tree-ssa-ccp.c      (working copy)
> @@ -1643,7 +1643,7 @@ maybe_fold_offset_to_array_ref (tree bas
>     is the desired result type.  */
>  /* ??? This doesn't handle class inheritance.  */
>
> -static tree
> +tree
>  maybe_fold_offset_to_component_ref (tree record_type, tree base, tree offset,
>                                     tree orig_type, bool base_is_ptr)
>  {
>

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

> 
> Yes.  As Andrew mentions, a few testcases would be nice.  And...
Sure, I had two in testing but wanted to wait for full regression runs.
It is:

/* { dg-do compile } */
/* { dg-options "-O1 -fdump-tree-optimized" } */
struct a{
	int a;
	int b;
} a;
int *
t()
{
	return (int *)&a;
}
/* { dg-final { scan-tree-dump "a.a" "optimized"} } */
/* { dg-final { cleanup-tree-dump "optimized" } } */


/* { dg-do compile } */
/* { dg-options "-O1 -fdump-tree-optimized" } */
struct a{
	char a;
	int b;
} a;
int *
t()
{
	return (((int *)&a)+1);
}
/* { dg-final { scan-tree-dump "a.b" "optimized"} } */
/* { dg-final { cleanup-tree-dump "optimized" } } */

It also shows one problem - with "int a" instead of "char a" in the
second testcase we and up with "a.a+4" since the first folder match
first.  Pretty weird, but it should not happen in the case of multiple
inheritance we are interested in since the individual fields should have
different types, right?

We probably could add folding for this (ie decode a.a into offset again,
and call function appropriately) if it seems worthwhile.  I guess it is
just weird side case.
> 
> especially for this one - does this really happen?  I guess the comment 
> should
> read (type *)(&A + offset) and here I would expect we fold it right to the 
> case
> below, (type *)&A + offset'.

Yes, it should be (type *)(&A + offset).
All three variants happens during the bootstrap.
I don't see folder that would be responsible for the conversion you
suggest above, but it definitly makes sense as canonicalization.

> 
> >       if ((TREE_CODE (op0) == MODIFY_EXPR
> >           || TREE_CODE (op0) == GIMPLE_MODIFY_STMT)
> >          && TREE_CONSTANT (GENERIC_TREE_OPERAND (op0, 1))
> >@@ -9227,6 +9284,15 @@ fold_binary (enum tree_code code, tree t
> >              if (tem)
> >                return fold_convert (type, tem);
> >            }
> >+          /* Convert ((type *)&A)+offset into 
> >&A->field_of_type_and_offset.  */
> >+         if (TREE_CODE (op0) == NOP_EXPR
> >+             && TREE_CODE (TREE_OPERAND (op0, 0)) == ADDR_EXPR
> >+             && TREE_CODE (op1) == INTEGER_CST
> >+             && (tem = maybe_fold_offset_to_component_ref
> >+                         (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (op0, 0), 
> >0)),
> >+                          TREE_OPERAND (TREE_OPERAND (op0, 0), 0),
> >+                          op1, TREE_TYPE (type), false)))
> >+           return build_fold_addr_expr_with_type (tem, type);
> 
> Do we handle &A + offset (without the cast) somewhere already in fold?

We don't, OK, since we just cummulated another at least 3 ideas for
folding, I am sending the easy part first (that deals only with first
testcase) and will do other three incrementally.

Thanks,
Honza

	* tree.h (maybe_fold_offset_to_component_ref): Declare.
	* fold-const.c (fold_unary): Fold NOPs of ADDR_EXPR
	into COMPONENT_REFs.
	* tree-ssa-ccp.c (maybe_fold_offset_to_component_ref): Export.
Index: tree.h
===================================================================
--- tree.h	(revision 123639)
+++ tree.h	(working copy)
@@ -4413,6 +4413,8 @@ extern void fold_defer_overflow_warnings
 extern void fold_undefer_overflow_warnings (bool, tree, int);
 extern void fold_undefer_and_ignore_overflow_warnings (void);
 extern bool fold_deferring_overflow_warnings_p (void);
+extern tree maybe_fold_offset_to_component_ref (tree, tree, tree,
+				                tree, bool);
 
 extern tree force_fit_type_double (tree, unsigned HOST_WIDE_INT, HOST_WIDE_INT,
 				   int, bool);
Index: tree-ssa-ccp.c
===================================================================
--- tree-ssa-ccp.c	(revision 123639)
+++ tree-ssa-ccp.c	(working copy)
@@ -1643,7 +1643,7 @@ maybe_fold_offset_to_array_ref (tree bas
    is the desired result type.  */
 /* ??? This doesn't handle class inheritance.  */
 
-static tree
+tree
 maybe_fold_offset_to_component_ref (tree record_type, tree base, tree offset,
 				    tree orig_type, bool base_is_ptr)
 {
Index: fold-const.c
===================================================================
*** fold-const.c	(revision 123639)
--- fold-const.c	(working copy)
*************** fold_view_convert_expr (tree type, tree 
*************** fold_unary (enum tree_code code, tree ty
*** 7735,7740 ****
--- 7775,7787 ----
  	    return fold_convert (type, build_fold_addr_expr (base));
          }
  
+       /* Convert (type *)&A into &A->field_of_type_and_offset_0.  */
+       if (TREE_CODE (op0) == ADDR_EXPR && POINTER_TYPE_P (type)
+ 	  && (tem = maybe_fold_offset_to_component_ref
+ 		      (TREE_TYPE (TREE_OPERAND (op0, 0)), TREE_OPERAND (op0, 0),
+ 		       integer_zero_node, TREE_TYPE (type), false)))
+         return build_fold_addr_expr_with_type (tem, type);
+ 
        if ((TREE_CODE (op0) == MODIFY_EXPR
  	   || TREE_CODE (op0) == GIMPLE_MODIFY_STMT)
  	  && TREE_CONSTANT (GENERIC_TREE_OPERAND (op0, 1))

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Richard Guenther]

On 4/8/07, Jan Hubicka <jh@suse.cz> wrote:
>
> >
> > Yes.  As Andrew mentions, a few testcases would be nice.  And...
> Sure, I had two in testing but wanted to wait for full regression runs.
> It is:
>
> /* { dg-do compile } */
> /* { dg-options "-O1 -fdump-tree-optimized" } */
> struct a{
>         int a;
>         int b;
> } a;
> int *
> t()
> {
>         return (int *)&a;
> }
> /* { dg-final { scan-tree-dump "a.a" "optimized"} } */
> /* { dg-final { cleanup-tree-dump "optimized" } } */
>
>
> /* { dg-do compile } */
> /* { dg-options "-O1 -fdump-tree-optimized" } */
> struct a{
>         char a;
>         int b;
> } a;
> int *
> t()
> {
>         return (((int *)&a)+1);
> }
> /* { dg-final { scan-tree-dump "a.b" "optimized"} } */
> /* { dg-final { cleanup-tree-dump "optimized" } } */
>
> It also shows one problem - with "int a" instead of "char a" in the
> second testcase we and up with "a.a+4" since the first folder match
> first.  Pretty weird, but it should not happen in the case of multiple
> inheritance we are interested in since the individual fields should have
> different types, right?
>
> We probably could add folding for this (ie decode a.a into offset again,
> and call function appropriately) if it seems worthwhile.  I guess it is
> just weird side case.
> >
> > especially for this one - does this really happen?  I guess the comment
> > should
> > read (type *)(&A + offset) and here I would expect we fold it right to the
> > case
> > below, (type *)&A + offset'.
>
> Yes, it should be (type *)(&A + offset).
> All three variants happens during the bootstrap.
> I don't see folder that would be responsible for the conversion you
> suggest above, but it definitly makes sense as canonicalization.
>
> >
> > >       if ((TREE_CODE (op0) == MODIFY_EXPR
> > >           || TREE_CODE (op0) == GIMPLE_MODIFY_STMT)
> > >          && TREE_CONSTANT (GENERIC_TREE_OPERAND (op0, 1))
> > >@@ -9227,6 +9284,15 @@ fold_binary (enum tree_code code, tree t
> > >              if (tem)
> > >                return fold_convert (type, tem);
> > >            }
> > >+          /* Convert ((type *)&A)+offset into
> > >&A->field_of_type_and_offset.  */
> > >+         if (TREE_CODE (op0) == NOP_EXPR
> > >+             && TREE_CODE (TREE_OPERAND (op0, 0)) == ADDR_EXPR
> > >+             && TREE_CODE (op1) == INTEGER_CST
> > >+             && (tem = maybe_fold_offset_to_component_ref
> > >+                         (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (op0, 0),
> > >0)),
> > >+                          TREE_OPERAND (TREE_OPERAND (op0, 0), 0),
> > >+                          op1, TREE_TYPE (type), false)))
> > >+           return build_fold_addr_expr_with_type (tem, type);
> >
> > Do we handle &A + offset (without the cast) somewhere already in fold?
>
> We don't, OK, since we just cummulated another at least 3 ideas for
> folding, I am sending the easy part first (that deals only with first
> testcase) and will do other three incrementally.

This is ok with after proper testing.

Thanks,
Richard.

> Thanks,
> Honza
>
>         * tree.h (maybe_fold_offset_to_component_ref): Declare.
>         * fold-const.c (fold_unary): Fold NOPs of ADDR_EXPR
>         into COMPONENT_REFs.
>         * tree-ssa-ccp.c (maybe_fold_offset_to_component_ref): Export.
> Index: tree.h
> ===================================================================
> --- tree.h      (revision 123639)
> +++ tree.h      (working copy)
> @@ -4413,6 +4413,8 @@ extern void fold_defer_overflow_warnings
>  extern void fold_undefer_overflow_warnings (bool, tree, int);
>  extern void fold_undefer_and_ignore_overflow_warnings (void);
>  extern bool fold_deferring_overflow_warnings_p (void);
> +extern tree maybe_fold_offset_to_component_ref (tree, tree, tree,
> +                                               tree, bool);
>
>  extern tree force_fit_type_double (tree, unsigned HOST_WIDE_INT, HOST_WIDE_INT,
>                                    int, bool);
> Index: tree-ssa-ccp.c
> ===================================================================
> --- tree-ssa-ccp.c      (revision 123639)
> +++ tree-ssa-ccp.c      (working copy)
> @@ -1643,7 +1643,7 @@ maybe_fold_offset_to_array_ref (tree bas
>     is the desired result type.  */
>  /* ??? This doesn't handle class inheritance.  */
>
> -static tree
> +tree
>  maybe_fold_offset_to_component_ref (tree record_type, tree base, tree offset,
>                                     tree orig_type, bool base_is_ptr)
>  {
> Index: fold-const.c
> ===================================================================
> *** fold-const.c        (revision 123639)
> --- fold-const.c        (working copy)
> *************** fold_view_convert_expr (tree type, tree
> *************** fold_unary (enum tree_code code, tree ty
> *** 7735,7740 ****
> --- 7775,7787 ----
>             return fold_convert (type, build_fold_addr_expr (base));
>           }
>
> +       /* Convert (type *)&A into &A->field_of_type_and_offset_0.  */
> +       if (TREE_CODE (op0) == ADDR_EXPR && POINTER_TYPE_P (type)
> +         && (tem = maybe_fold_offset_to_component_ref
> +                     (TREE_TYPE (TREE_OPERAND (op0, 0)), TREE_OPERAND (op0, 0),
> +                      integer_zero_node, TREE_TYPE (type), false)))
> +         return build_fold_addr_expr_with_type (tem, type);
> +
>         if ((TREE_CODE (op0) == MODIFY_EXPR
>            || TREE_CODE (op0) == GIMPLE_MODIFY_STMT)
>           && TREE_CONSTANT (GENERIC_TREE_OPERAND (op0, 1))
>

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

> >We don't, OK, since we just cummulated another at least 3 ideas for
> >folding, I am sending the easy part first (that deals only with first
> >testcase) and will do other three incrementally.

Hi,
this fixes the maybe_fold_offset_to_component_ref problem wrt recursion on
union types.  Now the testcase:
/* { dg-do compile } */
/* { dg-options "-O1 -fdump-tree-optimized" } */
union a
{
  struct s1
  {
    long long a;
    long long b;
  } s1;
  struct s2
  {
    int c;
    int d;
  } s2;
  struct s3
  {
    unsigned long long e;
    unsigned long long f;
  } s3;
} a;
int *
t ()
{
  return (int *) &a;
}

/* { dg-final { scan-tree-dump "a.s2.c" "optimized"} } */
/* { dg-final { cleanup-tree-dump "optimized" } } */

is hanled correctly. I am still bit concerned about the memory issues
from all the containers but at the moment it don't seem to be causing
problems (the C++ tester showed just slight improvement on tramp3d both
runtime and memory for my previous patch that should be about the most
expensive one).

It is not dificult at all to solve the memory by the array trick if
needed at later time.

Bootstrapped/regtested i686-linux, OK?

Honza

	* tree-ssa-ccp (maybe_fold_offset_to_component_ref): Recurse into
	multiple fields of union.
Index: tree-ssa-ccp.c
===================================================================
--- tree-ssa-ccp.c	(revision 123670)
+++ tree-ssa-ccp.c	(working copy)
@@ -1648,6 +1648,8 @@ maybe_fold_offset_to_component_ref (tree
 				    tree orig_type, bool base_is_ptr)
 {
   tree f, t, field_type, tail_array_field, field_offset;
+  tree ret;
+  tree new_base;
 
   if (TREE_CODE (record_type) != RECORD_TYPE
       && TREE_CODE (record_type) != UNION_TYPE
@@ -1719,8 +1721,20 @@ maybe_fold_offset_to_component_ref (tree
 
       /* If we matched, then set offset to the displacement into
 	 this field.  */
-      offset = t;
-      goto found;
+      if (base_is_ptr)
+	new_base = build1 (INDIRECT_REF, record_type, base);
+      else
+	new_base = base;
+      new_base = build3 (COMPONENT_REF, field_type, new_base, f, NULL_TREE);
+
+      /* Recurse to possibly find the match.  */
+      ret = maybe_fold_offset_to_array_ref (new_base, t, orig_type);
+      if (ret)
+	return ret;
+      ret = maybe_fold_offset_to_component_ref (field_type, new_base, t,
+						orig_type, false);
+      if (ret)
+	return ret;
     }
 
   if (!tail_array_field)
@@ -1730,7 +1744,6 @@ maybe_fold_offset_to_component_ref (tree
   field_type = TREE_TYPE (f);
   offset = int_const_binop (MINUS_EXPR, offset, byte_position (f), 1);
 
- found:
   /* If we get here, we've got an aggregate field, and a possibly 
      nonzero offset into them.  Recurse and hope for a valid match.  */
   if (base_is_ptr)

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Richard Guenther]

On 4/9/07, Jan Hubicka <jh@suse.cz> wrote:
> > >We don't, OK, since we just cummulated another at least 3 ideas for
> > >folding, I am sending the easy part first (that deals only with first
> > >testcase) and will do other three incrementally.
>
> Hi,
> this fixes the maybe_fold_offset_to_component_ref problem wrt recursion on
> union types.  Now the testcase:
> /* { dg-do compile } */
> /* { dg-options "-O1 -fdump-tree-optimized" } */
> union a
> {
>   struct s1
>   {
>     long long a;
>     long long b;
>   } s1;
>   struct s2
>   {
>     int c;
>     int d;
>   } s2;
>   struct s3
>   {
>     unsigned long long e;
>     unsigned long long f;
>   } s3;
> } a;
> int *
> t ()
> {
>   return (int *) &a;
> }
>
> /* { dg-final { scan-tree-dump "a.s2.c" "optimized"} } */
> /* { dg-final { cleanup-tree-dump "optimized" } } */
>
> is hanled correctly. I am still bit concerned about the memory issues
> from all the containers but at the moment it don't seem to be causing
> problems (the C++ tester showed just slight improvement on tramp3d both
> runtime and memory for my previous patch that should be about the most
> expensive one).
>
> It is not dificult at all to solve the memory by the array trick if
> needed at later time.
>
> Bootstrapped/regtested i686-linux, OK?

This is ok if you also add the testcase (it's missing from the ChangeLog).

Thanks,
Richard.

> Honza
>
>         * tree-ssa-ccp (maybe_fold_offset_to_component_ref): Recurse into
>         multiple fields of union.
> Index: tree-ssa-ccp.c
> ===================================================================
> --- tree-ssa-ccp.c      (revision 123670)
> +++ tree-ssa-ccp.c      (working copy)
> @@ -1648,6 +1648,8 @@ maybe_fold_offset_to_component_ref (tree
>                                     tree orig_type, bool base_is_ptr)
>  {
>    tree f, t, field_type, tail_array_field, field_offset;
> +  tree ret;
> +  tree new_base;
>
>    if (TREE_CODE (record_type) != RECORD_TYPE
>        && TREE_CODE (record_type) != UNION_TYPE
> @@ -1719,8 +1721,20 @@ maybe_fold_offset_to_component_ref (tree
>
>        /* If we matched, then set offset to the displacement into
>          this field.  */
> -      offset = t;
> -      goto found;
> +      if (base_is_ptr)
> +       new_base = build1 (INDIRECT_REF, record_type, base);
> +      else
> +       new_base = base;
> +      new_base = build3 (COMPONENT_REF, field_type, new_base, f, NULL_TREE);
> +
> +      /* Recurse to possibly find the match.  */
> +      ret = maybe_fold_offset_to_array_ref (new_base, t, orig_type);
> +      if (ret)
> +       return ret;
> +      ret = maybe_fold_offset_to_component_ref (field_type, new_base, t,
> +                                               orig_type, false);
> +      if (ret)
> +       return ret;
>      }
>
>    if (!tail_array_field)
> @@ -1730,7 +1744,6 @@ maybe_fold_offset_to_component_ref (tree
>    field_type = TREE_TYPE (f);
>    offset = int_const_binop (MINUS_EXPR, offset, byte_position (f), 1);
>
> - found:
>    /* If we get here, we've got an aggregate field, and a possibly
>       nonzero offset into them.  Recurse and hope for a valid match.  */
>    if (base_is_ptr)
>

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

> 
> This is ok if you also add the testcase (it's missing from the ChangeLog).

I plan so, I am usually writting changelogs for testcases at time of
comitting them (so name is fixed), but I can change the procedure.

There is comment in from of maybe_fold_offset_to_component_ref:

/* A subroutine of fold_stmt_r.  Attempts to fold *(S+O) to S.X.
   BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
   is the desired result type.  */
/* ??? This doesn't handle class inheritance.  */

Any idea what the comment about inheritance is about?

Just for a record, I was also looking a bit into the multiple
inheritance issues.  In the testcase attached we get:

        try
          {
            this->newfld = 124;
            operator<< (&cout, &"BetterTrgl destructor\n"[0]);
          }
        finally
          {
            D.29475 = (struct BetterTrgl *) 4;
            D.29476 = this + D.29475;
            D.29477 = (struct Triangle *) D.29476;
	  ...

The cast on 4 seems a bit curious, I would expect it to be already
folded into INTEGER_CST.

This statement is later transformed into:

  D.29489_5 = this_1(D) + 4B;
  D.29490_6 = (struct Triangle *) D.29489_5;

I guess we want to have this further folded into:

  D.29490_6 = &this_1(D)->Triangle;

There are also simple variant without the offset that would probably
also worth folding even if number of statements don't decrease
(ie  (partent_type *)ptr into &ptr->parent)

Looks like there are a lot more cases than I would like ;(
However if we fold this my testcase (with few iostreams and multiple
inheritance) would be mostly NOP_EXPR free.

Honza

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Richard Guenther]

On 4/9/07, Jan Hubicka <jh@suse.cz> wrote:
> >
> > This is ok if you also add the testcase (it's missing from the ChangeLog).
>
> I plan so, I am usually writting changelogs for testcases at time of
> comitting them (so name is fixed), but I can change the procedure.
>
> There is comment in from of maybe_fold_offset_to_component_ref:
>
> /* A subroutine of fold_stmt_r.  Attempts to fold *(S+O) to S.X.
>    BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
>    is the desired result type.  */
> /* ??? This doesn't handle class inheritance.  */
>
> Any idea what the comment about inheritance is about?

No idea.

> Just for a record, I was also looking a bit into the multiple
> inheritance issues.  In the testcase attached we get:
>
>         try
>           {
>             this->newfld = 124;
>             operator<< (&cout, &"BetterTrgl destructor\n"[0]);
>           }
>         finally
>           {
>             D.29475 = (struct BetterTrgl *) 4;
>             D.29476 = this + D.29475;
>             D.29477 = (struct Triangle *) D.29476;
>           ...
>
> The cast on 4 seems a bit curious, I would expect it to be already
> folded into INTEGER_CST.

Yes, it looks like somebody forgets to fold this.  (Inlining?)

> This statement is later transformed into:
>
>   D.29489_5 = this_1(D) + 4B;
>   D.29490_6 = (struct Triangle *) D.29489_5;
>
> I guess we want to have this further folded into:
>
>   D.29490_6 = &this_1(D)->Triangle;

Right.  Ideally we can use the tree combining the new value numberer
will do, but realistically that one still didn't arrive ;)

> There are also simple variant without the offset that would probably
> also worth folding even if number of statements don't decrease
> (ie  (partent_type *)ptr into &ptr->parent)

I think it's always worth removing a cast in favor of addressing the
right component.  Though if we succeed in creating a single
base + offset memory reference tree this work will be a wash if
the address is dereferenced...

> Looks like there are a lot more cases than I would like ;(
> However if we fold this my testcase (with few iostreams and multiple
> inheritance) would be mostly NOP_EXPR free.

It's worth fixing more testcases if they actually happen in real code - at
least that is what I have been doing in handling the (T*)&a->x.y to
&a->x case.

Richard.

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

> On 4/9/07, Jan Hubicka <jh@suse.cz> wrote:
> >>
> >> This is ok if you also add the testcase (it's missing from the 
> >ChangeLog).
> >
> >I plan so, I am usually writting changelogs for testcases at time of
> >comitting them (so name is fixed), but I can change the procedure.
> >
> >There is comment in from of maybe_fold_offset_to_component_ref:
> >
> >/* A subroutine of fold_stmt_r.  Attempts to fold *(S+O) to S.X.
> >   BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
> >   is the desired result type.  */
> >/* ??? This doesn't handle class inheritance.  */
> >
> >Any idea what the comment about inheritance is about?
> 
> No idea.
> >          {
> >            D.29475 = (struct BetterTrgl *) 4;
> >            D.29476 = this + D.29475;
> >            D.29477 = (struct Triangle *) D.29476;
> >          ...
> >
> >The cast on 4 seems a bit curious, I would expect it to be already
> >folded into INTEGER_CST.
> 
> Yes, it looks like somebody forgets to fold this.  (Inlining?)

It is pre-inlining (already in generic dump), probably it is result of
some gimplification, I will try to dig into.  In general I would however
preffer middle end to clean up this anyway.

> >There are also simple variant without the offset that would probably
> >also worth folding even if number of statements don't decrease
> >(ie  (partent_type *)ptr into &ptr->parent)
> 
> I think it's always worth removing a cast in favor of addressing the
> right component.  Though if we succeed in creating a single
> base + offset memory reference tree this work will be a wash if
> the address is dereferenced...

Yep, however I am bit more interested in cases this is not dereferenced
to help IPA optimizations Martin is working on.
> 
> >Looks like there are a lot more cases than I would like ;(
> >However if we fold this my testcase (with few iostreams and multiple
> >inheritance) would be mostly NOP_EXPR free.
> 
> It's worth fixing more testcases if they actually happen in real code - at
> least that is what I have been doing in handling the (T*)&a->x.y to
> &a->x case.

I am testing two extra cases and then I am apparently out of simple
C++ testcases.  I will probably take a look on few selected functions of
some bigger C++ body, but since my testcase does IO streams, I guess I
got the most common cases already.

Honza
> 
> Richard.

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

Hi,
this patch makes us to deal with the testcase:
/* { dg-do compile } */
/* { dg-options "-O1 -fdump-tree-optimized" } */
struct a{
	int a;
	int b;
} a;
int *
t()
{
	return (((int *)&a)+1);
}
/* { dg-final { scan-tree-dump "a.b" "optimized"} } */
/* { dg-final { cleanup-tree-dump "optimized" } } */

There is a need to add conversion of "((type *)&A)+offset" into
"&A->field_of_type_and_offset".  (the testcase is artifical, but this is also
what C++ constructs for multiple inheritance).  With this folding screws up and
ends up with &a.a+4B instead of &a.b by first simplyfing (type *)&A.  Adding a
code to remove containing COMPONENT_REFs into
maybe_fold_offset_to_component_ref_1 seems to be pretty effective adding couple
hounderd of new positives on compiling Gerald's testcase.

What still needs to be done is to add support for this transformation on gimple
level, probably into forward propagation, since number of cases seems to remain
unfolded until TER.

I've bootstrapped/regtested i686-linux with slightly different variant of
patch, I am re-running clean testing again. Ok assuming it passes?

Honza

	* gcc.dg/tree-ssa/foldaddr-4.c: New testcase.

	* fold-const.c (fold_binary): Convert ((type *)&A)+offset into
	&A->field_of_type_and_offset.
	* tree-ssa-ccp.c (maybe_fold_offset_to_component_ref_1): Rename from ...
	(maybe_fold_offset_to_component_ref): This one; when passed a COMPONENT_REF,
	try to eliminate it and combine the offsets.
Index: fold-const.c
===================================================================
--- fold-const.c	(revision 123674)
+++ fold-const.c	(working copy)
@@ -9234,6 +9249,15 @@ fold_binary (enum tree_code code, tree t
 	      if (tem)
 		return fold_convert (type, tem);
 	    }
+          /* Convert ((type *)&A)+offset into &A->field_of_type_and_offset.  */
+	  if (TREE_CODE (op0) == NOP_EXPR && POINTER_TYPE_P (type)
+	      && TREE_CODE (TREE_OPERAND (op0, 0)) == ADDR_EXPR
+	      && TREE_CODE (op1) == INTEGER_CST
+	      && (tem = maybe_fold_offset_to_component_ref
+			  (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (op0, 0), 0)),
+			   TREE_OPERAND (TREE_OPERAND (op0, 0), 0),
+			   op1, TREE_TYPE (type), false)))
+	    return build_fold_addr_expr_with_type (tem, type);
 	}
       else
 	{
Index: tree-ssa-ccp.c
===================================================================
--- tree-ssa-ccp.c	(revision 123674)
+++ tree-ssa-ccp.c	(working copy)
@@ -1637,15 +1637,13 @@ maybe_fold_offset_to_array_ref (tree bas
 			   / (TYPE_ALIGN_UNIT (elt_type))));
 }
 
-
-/* A subroutine of fold_stmt_r.  Attempts to fold *(S+O) to S.X.
+/* Attempt to fold *(S+O) to S.X.
    BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
-   is the desired result type.  */
-/* ??? This doesn't handle class inheritance.  */
-
-tree
-maybe_fold_offset_to_component_ref (tree record_type, tree base, tree offset,
-				    tree orig_type, bool base_is_ptr)
+   is the desired result type.  
+   Helper function for maybe_fold_offset_to_component_ref_1.  */
+static tree
+maybe_fold_offset_to_component_ref_1 (tree record_type, tree base, tree offset,
+				      tree orig_type, bool base_is_ptr)
 {
   tree f, t, field_type, tail_array_field, field_offset;
   tree ret;
@@ -1731,7 +1729,7 @@ maybe_fold_offset_to_component_ref (tree
       ret = maybe_fold_offset_to_array_ref (new_base, t, orig_type);
       if (ret)
 	return ret;
-      ret = maybe_fold_offset_to_component_ref (field_type, new_base, t,
+      ret = maybe_fold_offset_to_component_ref_1 (field_type, new_base, t,
 						orig_type, false);
       if (ret)
 	return ret;
@@ -1753,10 +1751,46 @@ maybe_fold_offset_to_component_ref (tree
   t = maybe_fold_offset_to_array_ref (base, offset, orig_type);
   if (t)
     return t;
-  return maybe_fold_offset_to_component_ref (field_type, base, offset,
-					     orig_type, false);
+  return maybe_fold_offset_to_component_ref_1 (field_type, base, offset,
+					       orig_type, false);
 }
 
+/* Attempt to fold *(S+O) to S.X.
+   BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
+   is the desired result type.  */
+/* ??? This doesn't handle class inheritance.  */
+
+tree
+maybe_fold_offset_to_component_ref (tree record_type, tree base, tree offset,
+				    tree orig_type, bool base_is_ptr)
+{
+  tree ret;
+  ret = maybe_fold_offset_to_component_ref_1 (record_type, base, offset,
+					      orig_type, base_is_ptr);
+
+  /* Handle case where existing COMPONENT_REF pick e.g. wrong field of union,
+     so it needs to be removed and new COMPONENT_REF constructed.
+     The wrong COMPONENT_REF are often constructed by folding the
+     (type *)&object within the expression (type *)&object+offset  */
+  if (!ret && !base_is_ptr && TREE_CODE (base) == COMPONENT_REF)
+    {
+      tree sub_base;
+      HOST_WIDE_INT sub_offset, size, maxsize;
+      sub_base = get_ref_base_and_extent (base, &sub_offset, &size, &maxsize);
+      if (sub_base == base || !sub_base)
+	ret = NULL_TREE;
+      else if (lang_hooks.types_compatible_p (TREE_TYPE (sub_base), orig_type))
+	ret = sub_base;
+      else
+	{
+	  offset = int_const_binop (PLUS_EXPR, offset,
+				    build_int_cst (TREE_TYPE (offset), sub_offset), 1);
+	  ret = maybe_fold_offset_to_component_ref_1 (TREE_TYPE (sub_base), sub_base, 
+						      offset, orig_type, false);
+	}
+    }
+  return ret;
+}
 
 /* A subroutine of fold_stmt_r.  Attempt to simplify *(BASE+OFFSET).
    Return the simplified expression, or NULL if nothing could be done.  */

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Richard Guenther]

On 4/9/07, Jan Hubicka <hubicka@ucw.cz> wrote:
> > On 4/9/07, Jan Hubicka <jh@suse.cz> wrote:
> > >>
> > >> This is ok if you also add the testcase (it's missing from the
> > >ChangeLog).
> > >
> > >I plan so, I am usually writting changelogs for testcases at time of
> > >comitting them (so name is fixed), but I can change the procedure.
> > >
> > >There is comment in from of maybe_fold_offset_to_component_ref:
> > >
> > >/* A subroutine of fold_stmt_r.  Attempts to fold *(S+O) to S.X.
> > >   BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
> > >   is the desired result type.  */
> > >/* ??? This doesn't handle class inheritance.  */
> > >
> > >Any idea what the comment about inheritance is about?
> >
> > No idea.
> > >          {
> > >            D.29475 = (struct BetterTrgl *) 4;
> > >            D.29476 = this + D.29475;
> > >            D.29477 = (struct Triangle *) D.29476;
> > >          ...
> > >
> > >The cast on 4 seems a bit curious, I would expect it to be already
> > >folded into INTEGER_CST.
> >
> > Yes, it looks like somebody forgets to fold this.  (Inlining?)
>
> It is pre-inlining (already in generic dump), probably it is result of
> some gimplification, I will try to dig into.  In general I would however
> preffer middle end to clean up this anyway.

Uh, no.  We have to keep the invariant that trees are always in folded
form.

Richard.

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

> On 4/9/07, Jan Hubicka <hubicka@ucw.cz> wrote:
> >> On 4/9/07, Jan Hubicka <jh@suse.cz> wrote:
> >> >>
> >> >> This is ok if you also add the testcase (it's missing from the
> >> >ChangeLog).
> >> >
> >> >I plan so, I am usually writting changelogs for testcases at time of
> >> >comitting them (so name is fixed), but I can change the procedure.
> >> >
> >> >There is comment in from of maybe_fold_offset_to_component_ref:
> >> >
> >> >/* A subroutine of fold_stmt_r.  Attempts to fold *(S+O) to S.X.
> >> >   BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
> >> >   is the desired result type.  */
> >> >/* ??? This doesn't handle class inheritance.  */
> >> >
> >> >Any idea what the comment about inheritance is about?
> >>
> >> No idea.
> >> >          {
> >> >            D.29475 = (struct BetterTrgl *) 4;
> >> >            D.29476 = this + D.29475;
> >> >            D.29477 = (struct Triangle *) D.29476;
> >> >          ...
> >> >
> >> >The cast on 4 seems a bit curious, I would expect it to be already
> >> >folded into INTEGER_CST.
> >>
> >> Yes, it looks like somebody forgets to fold this.  (Inlining?)
> >
> >It is pre-inlining (already in generic dump), probably it is result of
> >some gimplification, I will try to dig into.  In general I would however
> >preffer middle end to clean up this anyway.
> 
> Uh, no.  We have to keep the invariant that trees are always in folded
> form.

Sure (I even used to have verifier for that invariant which might be
interesting thing to rescuesce), however still we can construct those
testcases by other optimizations, so we ought to be able to fold them
both at gimplification time and at later time if they appear.

Honza
> 
> Richard.

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Richard Guenther]

On 4/9/07, Jan Hubicka <jh@suse.cz> wrote:
> Hi,
> this patch makes us to deal with the testcase:
> /* { dg-do compile } */
> /* { dg-options "-O1 -fdump-tree-optimized" } */
> struct a{
>         int a;
>         int b;
> } a;
> int *
> t()
> {
>         return (((int *)&a)+1);
> }
> /* { dg-final { scan-tree-dump "a.b" "optimized"} } */
> /* { dg-final { cleanup-tree-dump "optimized" } } */
>
> There is a need to add conversion of "((type *)&A)+offset" into
> "&A->field_of_type_and_offset".  (the testcase is artifical, but this is also
> what C++ constructs for multiple inheritance).  With this folding screws up and
> ends up with &a.a+4B instead of &a.b by first simplyfing (type *)&A.  Adding a
> code to remove containing COMPONENT_REFs into
> maybe_fold_offset_to_component_ref_1 seems to be pretty effective adding couple
> hounderd of new positives on compiling Gerald's testcase.
>
> What still needs to be done is to add support for this transformation on gimple
> level, probably into forward propagation, since number of cases seems to remain
> unfolded until TER.
>
> I've bootstrapped/regtested i686-linux with slightly different variant of
> patch, I am re-running clean testing again. Ok assuming it passes?
>
> Honza
>
>         * gcc.dg/tree-ssa/foldaddr-4.c: New testcase.
>
>         * fold-const.c (fold_binary): Convert ((type *)&A)+offset into
>         &A->field_of_type_and_offset.
>         * tree-ssa-ccp.c (maybe_fold_offset_to_component_ref_1): Rename from ...
>         (maybe_fold_offset_to_component_ref): This one; when passed a COMPONENT_REF,
>         try to eliminate it and combine the offsets.
> Index: fold-const.c
> ===================================================================
> --- fold-const.c        (revision 123674)
> +++ fold-const.c        (working copy)
> @@ -9234,6 +9249,15 @@ fold_binary (enum tree_code code, tree t
>               if (tem)
>                 return fold_convert (type, tem);
>             }
> +          /* Convert ((type *)&A)+offset into &A->field_of_type_and_offset.  */
> +         if (TREE_CODE (op0) == NOP_EXPR && POINTER_TYPE_P (type)
> +             && TREE_CODE (TREE_OPERAND (op0, 0)) == ADDR_EXPR
> +             && TREE_CODE (op1) == INTEGER_CST
> +             && (tem = maybe_fold_offset_to_component_ref
> +                         (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (op0, 0), 0)),
> +                          TREE_OPERAND (TREE_OPERAND (op0, 0), 0),
> +                          op1, TREE_TYPE (type), false)))
> +           return build_fold_addr_expr_with_type (tem, type);
>         }
>        else
>         {
> Index: tree-ssa-ccp.c
> ===================================================================
> --- tree-ssa-ccp.c      (revision 123674)
> +++ tree-ssa-ccp.c      (working copy)
> @@ -1637,15 +1637,13 @@ maybe_fold_offset_to_array_ref (tree bas
>                            / (TYPE_ALIGN_UNIT (elt_type))));
>  }
>
> -
> -/* A subroutine of fold_stmt_r.  Attempts to fold *(S+O) to S.X.
> +/* Attempt to fold *(S+O) to S.X.
>     BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
> -   is the desired result type.  */
> -/* ??? This doesn't handle class inheritance.  */
> -
> -tree
> -maybe_fold_offset_to_component_ref (tree record_type, tree base, tree offset,
> -                                   tree orig_type, bool base_is_ptr)
> +   is the desired result type.
> +   Helper function for maybe_fold_offset_to_component_ref_1.  */
> +static tree
> +maybe_fold_offset_to_component_ref_1 (tree record_type, tree base, tree offset,
> +                                     tree orig_type, bool base_is_ptr)
>  {
>    tree f, t, field_type, tail_array_field, field_offset;
>    tree ret;
> @@ -1731,7 +1729,7 @@ maybe_fold_offset_to_component_ref (tree
>        ret = maybe_fold_offset_to_array_ref (new_base, t, orig_type);
>        if (ret)
>         return ret;
> -      ret = maybe_fold_offset_to_component_ref (field_type, new_base, t,
> +      ret = maybe_fold_offset_to_component_ref_1 (field_type, new_base, t,
>                                                 orig_type, false);
>        if (ret)
>         return ret;
> @@ -1753,10 +1751,46 @@ maybe_fold_offset_to_component_ref (tree
>    t = maybe_fold_offset_to_array_ref (base, offset, orig_type);
>    if (t)
>      return t;
> -  return maybe_fold_offset_to_component_ref (field_type, base, offset,
> -                                            orig_type, false);
> +  return maybe_fold_offset_to_component_ref_1 (field_type, base, offset,
> +                                              orig_type, false);
>  }
>
> +/* Attempt to fold *(S+O) to S.X.
> +   BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
> +   is the desired result type.  */
> +/* ??? This doesn't handle class inheritance.  */
> +
> +tree
> +maybe_fold_offset_to_component_ref (tree record_type, tree base, tree offset,
> +                                   tree orig_type, bool base_is_ptr)
> +{
> +  tree ret;
> +  ret = maybe_fold_offset_to_component_ref_1 (record_type, base, offset,
> +                                             orig_type, base_is_ptr);
> +
> +  /* Handle case where existing COMPONENT_REF pick e.g. wrong field of union,
> +     so it needs to be removed and new COMPONENT_REF constructed.
> +     The wrong COMPONENT_REF are often constructed by folding the
> +     (type *)&object within the expression (type *)&object+offset  */
> +  if (!ret && !base_is_ptr && TREE_CODE (base) == COMPONENT_REF)
> +    {
> +      tree sub_base;
> +      HOST_WIDE_INT sub_offset, size, maxsize;
> +      sub_base = get_ref_base_and_extent (base, &sub_offset, &size, &maxsize);
> +      if (sub_base == base || !sub_base)
> +       ret = NULL_TREE;
> +      else if (lang_hooks.types_compatible_p (TREE_TYPE (sub_base), orig_type))
> +       ret = sub_base;
> +      else
> +       {
> +         offset = int_const_binop (PLUS_EXPR, offset,
> +                                   build_int_cst (TREE_TYPE (offset), sub_offset), 1);
> +         ret = maybe_fold_offset_to_component_ref_1 (TREE_TYPE (sub_base), sub_base,
> +                                                     offset, orig_type, false);
> +       }
> +    }
> +  return ret;
> +}

Hmm, why not always use the canonical base to do this transformation?
You'd loose array refs this way as well, so you'd have to dispatch to
maybe_fold_offset_to_array_ref if base is an array.  So you would create
an entry point to either of them via

tree
maybe_fold_offset_to_reference (tree type, tree base, tree offset)
{
  new_base = get_ref_base_and_extent (.....)
  if (new_base)
    {
       base = new_base;
       offset = offset + new_offset;
    }
  if (TREE_CODE (TREE_TYPE (base)) == ARRAY_TYPE)
    maybe_fold_offset_to_array_ref (...)
  else
    maybe_fold_offset_to_component_ref (...)
}

?

Richard.

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Richard Guenther]

On 4/9/07, Jan Hubicka <hubicka@ucw.cz> wrote:
> > On 4/9/07, Jan Hubicka <hubicka@ucw.cz> wrote:
> > >> On 4/9/07, Jan Hubicka <jh@suse.cz> wrote:
> > >> >>
> > >> >> This is ok if you also add the testcase (it's missing from the
> > >> >ChangeLog).
> > >> >
> > >> >I plan so, I am usually writting changelogs for testcases at time of
> > >> >comitting them (so name is fixed), but I can change the procedure.
> > >> >
> > >> >There is comment in from of maybe_fold_offset_to_component_ref:
> > >> >
> > >> >/* A subroutine of fold_stmt_r.  Attempts to fold *(S+O) to S.X.
> > >> >   BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
> > >> >   is the desired result type.  */
> > >> >/* ??? This doesn't handle class inheritance.  */
> > >> >
> > >> >Any idea what the comment about inheritance is about?
> > >>
> > >> No idea.
> > >> >          {
> > >> >            D.29475 = (struct BetterTrgl *) 4;
> > >> >            D.29476 = this + D.29475;
> > >> >            D.29477 = (struct Triangle *) D.29476;
> > >> >          ...
> > >> >
> > >> >The cast on 4 seems a bit curious, I would expect it to be already
> > >> >folded into INTEGER_CST.
> > >>
> > >> Yes, it looks like somebody forgets to fold this.  (Inlining?)
> > >
> > >It is pre-inlining (already in generic dump), probably it is result of
> > >some gimplification, I will try to dig into.  In general I would however
> > >preffer middle end to clean up this anyway.
> >
> > Uh, no.  We have to keep the invariant that trees are always in folded
> > form.
>
> Sure (I even used to have verifier for that invariant which might be
> interesting thing to rescuesce), however still we can construct those
> testcases by other optimizations, so we ought to be able to fold them
> both at gimplification time and at later time if they appear.

Well, I would expect that if you call fold() on (Foo *)4 that we already
fold it.  AFWIW

   D.29475 = (stuct BetterTrgl *) 4;

shouldn't be considered valid GIMPLE.

Richard.

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

> 
> Hmm, why not always use the canonical base to do this transformation?
> You'd loose array refs this way as well, so you'd have to dispatch to
> maybe_fold_offset_to_array_ref if base is an array.  So you would create
> an entry point to either of them via

I was concerned about two things - first get_ref_base_and_extend can
handle more of containers (such as REALPART_EXPR) and for large chain of
COMPONENT_REF we would end up reconstructing them from scratch quite
busilly with quadratic memory consumption overall.

Hmm, I guess it is not that big deal and I like it because I can also
commonize the ADDR_EXPR and non-ADDR_EXPR handling, so I will prepare
updated patch.

Honza
> 
> tree
> maybe_fold_offset_to_reference (tree type, tree base, tree offset)
> {
>  new_base = get_ref_base_and_extent (.....)
>  if (new_base)
>    {
>       base = new_base;
>       offset = offset + new_offset;
>    }
>  if (TREE_CODE (TREE_TYPE (base)) == ARRAY_TYPE)
>    maybe_fold_offset_to_array_ref (...)
>  else
>    maybe_fold_offset_to_component_ref (...)
> }
> 
> ?
> 
> Richard.

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

> >Sure (I even used to have verifier for that invariant which might be
> >interesting thing to rescuesce), however still we can construct those
> >testcases by other optimizations, so we ought to be able to fold them
> >both at gimplification time and at later time if they appear.
> 
> Well, I would expect that if you call fold() on (Foo *)4 that we already
> fold it.  AFWIW
> 
>   D.29475 = (stuct BetterTrgl *) 4;
> 
> shouldn't be considered valid GIMPLE.

Uh sure.  I was thinking of the whole expression, I am in progress of
tracking this down.

Here is updated patch I ended up with and I am testing now (i686-linux)
that should handle all interesting testcases that don't need help of
forward propagation in the testcase quoted above.

It has maybe_fold_offset_to_component_ref that is responsible for
removing possible NOPs, ADDR_EXPRs and containing COMPONENT_REFs.
Now fold-const can just call it for the generic case of (type *)A
and A+offset.

I've also added conversion of (T1)(X op Y) into ((T1)X op Y)
as discussed earlier so I don't have to specifically handle
(type)(A+offset) as mentioned earlier.  I am not quite sure how generic
we want to make this conversion (in general if one of the two operands
allows folding conversion away, I would say that the conversion is a
win).  For a moment I restricted it for a pointers and constant operand
that is always cheap to retype, since I am not sure how precisely define
that the conversion folds away.

Honza

	* tree.h (maybe_fold_offset_to_component_ref): Remove prototype.
	(maybe_fold_offset_to_reference): Add.
	* fold-const.c (fold_unary): Use maybe_fold_offset_to_component_ref
	to convert (type *)A into &A->field_of_type_and_offset_0.
	Fold (T1)(X op Y) into ((T1)X op Y), for pointer type T1 and
	T2 being a constant.
	(fold_binary): Convert A+offset into &A->field_of_offset.
	* tree-ssa-ccp.c (maybe_fold_offset_to_component_ref): Update comments.
	(maybe_fold_offset_to_reference): New function.
Index: tree.h
===================================================================
*** tree.h	(revision 123674)
--- tree.h	(working copy)
*************** extern void fold_defer_overflow_warnings
*** 4412,4419 ****
  extern void fold_undefer_overflow_warnings (bool, tree, int);
  extern void fold_undefer_and_ignore_overflow_warnings (void);
  extern bool fold_deferring_overflow_warnings_p (void);
! extern tree maybe_fold_offset_to_component_ref (tree, tree, tree,
! 				                tree, bool);
  
  extern tree force_fit_type_double (tree, unsigned HOST_WIDE_INT, HOST_WIDE_INT,
  				   int, bool);
--- 4412,4418 ----
  extern void fold_undefer_overflow_warnings (bool, tree, int);
  extern void fold_undefer_and_ignore_overflow_warnings (void);
  extern bool fold_deferring_overflow_warnings_p (void);
! extern tree maybe_fold_offset_to_reference (tree, tree, tree);
  
  extern tree force_fit_type_double (tree, unsigned HOST_WIDE_INT, HOST_WIDE_INT,
  				   int, bool);
Index: fold-const.c
===================================================================
*** fold-const.c	(revision 123674)
--- fold-const.c	(working copy)
*************** fold_unary (enum tree_code code, tree ty
*** 7735,7745 ****
  	    return fold_convert (type, build_fold_addr_expr (base));
          }
  
!       /* Convert (type *)&A into &A->field_of_type_and_offset_0.  */
!       if (TREE_CODE (op0) == ADDR_EXPR && POINTER_TYPE_P (type)
! 	  && (tem = maybe_fold_offset_to_component_ref
! 		      (TREE_TYPE (TREE_OPERAND (op0, 0)), TREE_OPERAND (op0, 0),
! 		       integer_zero_node, TREE_TYPE (type), false)))
          return build_fold_addr_expr_with_type (tem, type);
  
        if ((TREE_CODE (op0) == MODIFY_EXPR
--- 7735,7744 ----
  	    return fold_convert (type, build_fold_addr_expr (base));
          }
  
!       /* Convert (type *)A into &A->field_of_type_and_offset_0.  */
!       if (POINTER_TYPE_P (type) && POINTER_TYPE_P (TREE_TYPE (op0))
! 	  && (tem = maybe_fold_offset_to_reference
! 		      (op0, integer_zero_node, TREE_TYPE (type))))
          return build_fold_addr_expr_with_type (tem, type);
  
        if ((TREE_CODE (op0) == MODIFY_EXPR
*************** fold_unary (enum tree_code code, tree ty
*** 7828,7833 ****
--- 7827,7845 ----
  			   TREE_OPERAND (arg0, 1));
  	}
  
+       /* Convert (T1)(X op Y) into ((T1)X op Y), for pointer types T1 and
+ 	 T2 being a constant, so retyping being cheap.  */
+       if (POINTER_TYPE_P (type)
+ 	  && BINARY_CLASS_P (arg0)
+ 	  && TREE_CODE (TREE_OPERAND (arg0, 1)) == INTEGER_CST)
+ 	{
+ 	  tree arg00 = TREE_OPERAND (arg0, 0);
+ 	  tree arg01 = TREE_OPERAND (arg0, 1);
+ 
+ 	  return fold_build2 (TREE_CODE (arg0), type, fold_convert (type, arg00),
+ 			      fold_convert (type, arg01));
+ 	}
+ 
        /* Convert (T1)(~(T2)X) into ~(T1)X if T1 and T2 are integral types
  	 of the same precision, and X is a integer type not narrower than
  	 types T1 or T2, i.e. the cast (T2)X isn't an extension.  */
*************** fold_binary (enum tree_code code, tree t
*** 9234,9239 ****
--- 9246,9256 ----
  	      if (tem)
  		return fold_convert (type, tem);
  	    }
+           /* Convert (A+offset into &A->field_of_type_and_offset.  */
+ 	  if (POINTER_TYPE_P (type) && TREE_CODE (op1) == INTEGER_CST
+ 	      && (tem = maybe_fold_offset_to_reference (op0, op1,
+ 			      				TREE_TYPE (type))))
+              return build_fold_addr_expr_with_type (tem, type);
  	}
        else
  	{
Index: tree-ssa-ccp.c
===================================================================
*** tree-ssa-ccp.c	(revision 123674)
--- tree-ssa-ccp.c	(working copy)
*************** maybe_fold_offset_to_array_ref (tree bas
*** 1637,1651 ****
  			   / (TYPE_ALIGN_UNIT (elt_type))));
  }
  
! 
! /* A subroutine of fold_stmt_r.  Attempts to fold *(S+O) to S.X.
     BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
!    is the desired result type.  */
! /* ??? This doesn't handle class inheritance.  */
  
! tree
  maybe_fold_offset_to_component_ref (tree record_type, tree base, tree offset,
! 				    tree orig_type, bool base_is_ptr)
  {
    tree f, t, field_type, tail_array_field, field_offset;
    tree ret;
--- 1637,1649 ----
  			   / (TYPE_ALIGN_UNIT (elt_type))));
  }
  
! /* Attempt to fold *(S+O) to S.X.
     BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
!    is the desired result type.   */
  
! static tree
  maybe_fold_offset_to_component_ref (tree record_type, tree base, tree offset,
! 				      tree orig_type, bool base_is_ptr)
  {
    tree f, t, field_type, tail_array_field, field_offset;
    tree ret;
*************** maybe_fold_offset_to_component_ref (tree
*** 1754,1762 ****
    if (t)
      return t;
    return maybe_fold_offset_to_component_ref (field_type, base, offset,
! 					     orig_type, false);
  }
  
  
  /* A subroutine of fold_stmt_r.  Attempt to simplify *(BASE+OFFSET).
     Return the simplified expression, or NULL if nothing could be done.  */
--- 1752,1816 ----
    if (t)
      return t;
    return maybe_fold_offset_to_component_ref (field_type, base, offset,
! 					       orig_type, false);
  }
  
+ /* Attempt to express (ORIG_TYPE)BASE+OFFSET as BASE->field_of_orig_type
+    or BASE[index] or by combination of those. 
+ 
+    Before attempting the conversion strip off existing ADDR_EXPRs and
+    handled component refs.  */
+ 
+ tree
+ maybe_fold_offset_to_reference (tree base, tree offset, tree orig_type)
+ {
+   tree ret;
+   tree type;
+   bool base_is_ptr = true;
+ 
+   STRIP_NOPS (base);
+   if (TREE_CODE (base) == ADDR_EXPR)
+     {
+       base_is_ptr = false;
+ 
+       base = TREE_OPERAND (base, 0);
+ 
+       /* Handle case where existing COMPONENT_REF pick e.g. wrong field of union,
+ 	 so it needs to be removed and new COMPONENT_REF constructed.
+ 	 The wrong COMPONENT_REF are often constructed by folding the
+ 	 (type *)&object within the expression (type *)&object+offset  */
+       if (handled_component_p (base))
+ 	{
+           HOST_WIDE_INT sub_offset, size, maxsize;
+ 	  base = get_ref_base_and_extent (base, &sub_offset,
+ 					  &size, &maxsize);
+ 	  gcc_assert (base);
+ 	  if (sub_offset)
+ 	    offset = int_const_binop (PLUS_EXPR, offset,
+ 				      build_int_cst (TREE_TYPE (offset), sub_offset), 1);
+ 	}
+       if (lang_hooks.types_compatible_p (TREE_TYPE (base), orig_type)
+ 	  && integer_zerop (offset))
+ 	return base;
+       type = TREE_TYPE (base);
+     }
+   else
+     {
+       base_is_ptr = true;
+       if (!POINTER_TYPE_P (base))
+ 	return NULL_TREE;
+       type = TREE_TYPE (TREE_TYPE (base));
+     }
+   ret = maybe_fold_offset_to_component_ref (type, base, offset,
+ 					    orig_type, base_is_ptr);
+   if (!ret)
+     {
+       if (base_is_ptr)
+ 	base = build1 (INDIRECT_REF, type, base);
+       ret = maybe_fold_offset_to_array_ref (base, offset, orig_type);
+     }
+   return ret;
+ }
  
  /* A subroutine of fold_stmt_r.  Attempt to simplify *(BASE+OFFSET).
     Return the simplified expression, or NULL if nothing could be done.  */

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Richard Guenther]

On 4/9/07, Jan Hubicka <jh@suse.cz> wrote:
> > >Sure (I even used to have verifier for that invariant which might be
> > >interesting thing to rescuesce), however still we can construct those
> > >testcases by other optimizations, so we ought to be able to fold them
> > >both at gimplification time and at later time if they appear.
> >
> > Well, I would expect that if you call fold() on (Foo *)4 that we already
> > fold it.  AFWIW
> >
> >   D.29475 = (stuct BetterTrgl *) 4;
> >
> > shouldn't be considered valid GIMPLE.
>
> Uh sure.  I was thinking of the whole expression, I am in progress of
> tracking this down.
>
> Here is updated patch I ended up with and I am testing now (i686-linux)
> that should handle all interesting testcases that don't need help of
> forward propagation in the testcase quoted above.
>
> It has maybe_fold_offset_to_component_ref that is responsible for
> removing possible NOPs, ADDR_EXPRs and containing COMPONENT_REFs.
> Now fold-const can just call it for the generic case of (type *)A
> and A+offset.
>
> I've also added conversion of (T1)(X op Y) into ((T1)X op Y)
> as discussed earlier so I don't have to specifically handle
> (type)(A+offset) as mentioned earlier.  I am not quite sure how generic
> we want to make this conversion (in general if one of the two operands
> allows folding conversion away, I would say that the conversion is a
> win).  For a moment I restricted it for a pointers and constant operand
> that is always cheap to retype, since I am not sure how precisely define
> that the conversion folds away.

This looks good.  Maybe you can merge it with

      /* Convert (T1)((T2)X op Y) into (T1)X op Y, for pointer types T1 and
         T2 being pointers to types of the same size.  */
      if (POINTER_TYPE_P (type)
          && BINARY_CLASS_P (arg0)
          && TREE_CODE (TREE_OPERAND (arg0, 0)) == NOP_EXPR
          && POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (arg0, 0))))
        {
          tree arg00 = TREE_OPERAND (arg0, 0);
          tree t0 = type;
          tree t1 = TREE_TYPE (arg00);
          tree tt0 = TREE_TYPE (t0);
          tree tt1 = TREE_TYPE (t1);
          tree s0 = TYPE_SIZE (tt0);
          tree s1 = TYPE_SIZE (tt1);

          if (s0 && s1 && operand_equal_p (s0, s1, OEP_ONLY_CONST))
            return build2 (TREE_CODE (arg0), t0, fold_convert (t0, arg00),
                           TREE_OPERAND (arg0, 1));
        }

and thereby try to decipher why we care about pointed to size here ;)
The build2 also lacks a fold_ and TREE_OPERAND (arg0, 1) needs
to be fold_converted to t0 as well.  Maybe you can "fix" these issues
as you are in the neighbourhood ;)

Otherwise this is ok if it passes testing.

+           /* Convert (A+offset into &A->field_of_type_and_offset.  */

parenthesis mismatch.

Thanks,
Richard.

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

> On 4/9/07, Jan Hubicka <jh@suse.cz> wrote:
> >> >Sure (I even used to have verifier for that invariant which might be
> >> >interesting thing to rescuesce), however still we can construct those
> >> >testcases by other optimizations, so we ought to be able to fold them
> >> >both at gimplification time and at later time if they appear.
> >>
> >> Well, I would expect that if you call fold() on (Foo *)4 that we already
> >> fold it.  AFWIW
> >>
> >>   D.29475 = (stuct BetterTrgl *) 4;
> >>
> >> shouldn't be considered valid GIMPLE.
> >
> >Uh sure.  I was thinking of the whole expression, I am in progress of
> >tracking this down.
> >
> >Here is updated patch I ended up with and I am testing now (i686-linux)
> >that should handle all interesting testcases that don't need help of
> >forward propagation in the testcase quoted above.
> >
> >It has maybe_fold_offset_to_component_ref that is responsible for
> >removing possible NOPs, ADDR_EXPRs and containing COMPONENT_REFs.
> >Now fold-const can just call it for the generic case of (type *)A
> >and A+offset.
> >
> >I've also added conversion of (T1)(X op Y) into ((T1)X op Y)
> >as discussed earlier so I don't have to specifically handle
> >(type)(A+offset) as mentioned earlier.  I am not quite sure how generic
> >we want to make this conversion (in general if one of the two operands
> >allows folding conversion away, I would say that the conversion is a
> >win).  For a moment I restricted it for a pointers and constant operand
> >that is always cheap to retype, since I am not sure how precisely define
> >that the conversion folds away.
> 
> This looks good.  Maybe you can merge it with
> 
>      /* Convert (T1)((T2)X op Y) into (T1)X op Y, for pointer types T1 and
>         T2 being pointers to types of the same size.  */
>      if (POINTER_TYPE_P (type)
>          && BINARY_CLASS_P (arg0)
>          && TREE_CODE (TREE_OPERAND (arg0, 0)) == NOP_EXPR
>          && POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (arg0, 0))))
>        {
>          tree arg00 = TREE_OPERAND (arg0, 0);
>          tree t0 = type;
>          tree t1 = TREE_TYPE (arg00);
>          tree tt0 = TREE_TYPE (t0);
>          tree tt1 = TREE_TYPE (t1);
>          tree s0 = TYPE_SIZE (tt0);
>          tree s1 = TYPE_SIZE (tt1);
> 
>          if (s0 && s1 && operand_equal_p (s0, s1, OEP_ONLY_CONST))
>            return build2 (TREE_CODE (arg0), t0, fold_convert (t0, arg00),
>                           TREE_OPERAND (arg0, 1));
>        }
> 
> and thereby try to decipher why we care about pointed to size here ;)
> The build2 also lacks a fold_ and TREE_OPERAND (arg0, 1) needs
> to be fold_converted to t0 as well.  Maybe you can "fix" these issues
> as you are in the neighbourhood ;)

Hmm, maybe I can genericise the conversion for case when second operand
is INTEGER_CST or one of operands is NOP_EXPR that will get folded away
then and remove the case above.
I can't think of any reason why one would require checking the size
except for the case that someone was thinking of C pointer + operator
that is multiplied by the pointed to size...
> 
> Otherwise this is ok if it passes testing.
> 
> +           /* Convert (A+offset into &A->field_of_type_and_offset.  */
> 
> parenthesis mismatch.

Uh, I will fix it and commit without the hunk handling the conversion
above and will send updated patch for that.

Thanks!
Honza
> 
> Thanks,
> Richard.

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

> 
> Hmm, maybe I can genericise the conversion for case when second operand
> is INTEGER_CST or one of operands is NOP_EXPR that will get folded away
> then and remove the case above.
> I can't think of any reason why one would require checking the size
> except for the case that someone was thinking of C pointer + operator
> that is multiplied by the pointed to size...
> > 
> > Otherwise this is ok if it passes testing.
> > 
> > +           /* Convert (A+offset into &A->field_of_type_and_offset.  */
> > 
> > parenthesis mismatch.
> 
> Uh, I will fix it and commit without the hunk handling the conversion
> above and will send updated patch for that.
Hi,
here is the updated patch, bootstrapped/regtested i686-linux, OK?

	* fold-const.c (fold_unary): Convert (T1)(X op Y) into
	((T1)X op (T1)Y), for pointer type, when one of the new casts will fold
	away.
Index: fold-const.c
===================================================================
*** fold-const.c	(revision 123674)
--- fold-const.c	(working copy)
*************** fold_unary (enum tree_code code, tree ty
*** 7808,7831 ****
  	    }
  	}
  
!       /* Convert (T1)((T2)X op Y) into (T1)X op Y, for pointer types T1 and
! 	 T2 being pointers to types of the same size.  */
!       if (POINTER_TYPE_P (type)
  	  && BINARY_CLASS_P (arg0)
! 	  && TREE_CODE (TREE_OPERAND (arg0, 0)) == NOP_EXPR
! 	  && POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (arg0, 0))))
  	{
  	  tree arg00 = TREE_OPERAND (arg0, 0);
! 	  tree t0 = type;
! 	  tree t1 = TREE_TYPE (arg00);
! 	  tree tt0 = TREE_TYPE (t0);
! 	  tree tt1 = TREE_TYPE (t1);
! 	  tree s0 = TYPE_SIZE (tt0);
! 	  tree s1 = TYPE_SIZE (tt1);
! 
! 	  if (s0 && s1 && operand_equal_p (s0, s1, OEP_ONLY_CONST))
! 	    return build2 (TREE_CODE (arg0), t0, fold_convert (t0, arg00),
! 			   TREE_OPERAND (arg0, 1));
  	}
  
        /* Convert (T1)(~(T2)X) into ~(T1)X if T1 and T2 are integral types
--- 7807,7826 ----
  	    }
  	}
  
!       /* Convert (T1)(X op Y) into ((T1)X op (T1)Y), for pointer type,
!          when one of the new casts will fold away. Conservatively we assume
! 	 that this happens when X or Y is NOP_EXPR or Y is INTEGER_CST.  */
!       if (POINTER_TYPE_P (type) && POINTER_TYPE_P (arg0)
  	  && BINARY_CLASS_P (arg0)
! 	  && (TREE_CODE (TREE_OPERAND (arg0, 1)) == INTEGER_CST
! 	      || TREE_CODE (TREE_OPERAND (arg0, 0)) == NOP_EXPR
! 	      || TREE_CODE (TREE_OPERAND (arg0, 1)) == NOP_EXPR))
  	{
  	  tree arg00 = TREE_OPERAND (arg0, 0);
! 	  tree arg01 = TREE_OPERAND (arg0, 1);
! 
! 	  return fold_build2 (TREE_CODE (arg0), type, fold_convert (type, arg00),
! 			      fold_convert (type, arg01));
  	}
  
        /* Convert (T1)(~(T2)X) into ~(T1)X if T1 and T2 are integral types

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

> On 4/9/07, Jan Hubicka <hubicka@ucw.cz> wrote:
> >> On 4/9/07, Jan Hubicka <jh@suse.cz> wrote:
> >> >>
> >> >> This is ok if you also add the testcase (it's missing from the
> >> >ChangeLog).
> >> >
> >> >I plan so, I am usually writting changelogs for testcases at time of
> >> >comitting them (so name is fixed), but I can change the procedure.
> >> >
> >> >There is comment in from of maybe_fold_offset_to_component_ref:
> >> >
> >> >/* A subroutine of fold_stmt_r.  Attempts to fold *(S+O) to S.X.
> >> >   BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
> >> >   is the desired result type.  */
> >> >/* ??? This doesn't handle class inheritance.  */
> >> >
> >> >Any idea what the comment about inheritance is about?
> >>
> >> No idea.
> >> >          {
> >> >            D.29475 = (struct BetterTrgl *) 4;
> >> >            D.29476 = this + D.29475;
> >> >            D.29477 = (struct Triangle *) D.29476;
> >> >          ...
> >> >
> >> >The cast on 4 seems a bit curious, I would expect it to be already
> >> >folded into INTEGER_CST.
> >>
> >> Yes, it looks like somebody forgets to fold this.  (Inlining?)
> >
> >It is pre-inlining (already in generic dump), probably it is result of
> >some gimplification, I will try to dig into.  In general I would however
> >preffer middle end to clean up this anyway.
> 
> Uh, no.  We have to keep the invariant that trees are always in folded
> form.
Hi,
I've found source of my unfolded statement:

Index: gcc/cp/class.c
===================================================================
--- gcc/cp/class.c	(revision 123674)
+++ gcc/cp/class.c	(working copy)
@@ -522,10 +522,10 @@ convert_to_base_statically (tree expr, t
       pointer_type = build_pointer_type (expr_type);
       expr = build_unary_op (ADDR_EXPR, expr, /*noconvert=*/1);
       if (!integer_zerop (BINFO_OFFSET (base)))
-	  expr = build2 (PLUS_EXPR, pointer_type, expr,
-			 build_nop (pointer_type, BINFO_OFFSET (base)));
-      expr = build_nop (build_pointer_type (BINFO_TYPE (base)), expr);
-      expr = build1 (INDIRECT_REF, BINFO_TYPE (base), expr);
+	  expr = fold_build2 (PLUS_EXPR, pointer_type, expr,
+			      fold_convert (pointer_type, BINFO_OFFSET (base)));
+      expr = fold_convert (build_pointer_type (BINFO_TYPE (base)), expr);
+      expr = build_fold_indirect_ref (expr);
     }
 
   return expr;

I am testing the patch, but it seems like it was unfolded for a purpose.
Mark, can you enlighten me why this expression is not folded?
The (type)integer_cst seems particularly silly in such a common
construct.

Honza

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Mark Mitchell]

Jan Hubicka wrote:

> Index: gcc/cp/class.c
> ===================================================================
> --- gcc/cp/class.c	(revision 123674)
> +++ gcc/cp/class.c	(working copy)
> @@ -522,10 +522,10 @@ convert_to_base_statically (tree expr, t
>        pointer_type = build_pointer_type (expr_type);
>        expr = build_unary_op (ADDR_EXPR, expr, /*noconvert=*/1);
>        if (!integer_zerop (BINFO_OFFSET (base)))
> -	  expr = build2 (PLUS_EXPR, pointer_type, expr,
> -			 build_nop (pointer_type, BINFO_OFFSET (base)));
> -      expr = build_nop (build_pointer_type (BINFO_TYPE (base)), expr);
> -      expr = build1 (INDIRECT_REF, BINFO_TYPE (base), expr);
> +	  expr = fold_build2 (PLUS_EXPR, pointer_type, expr,
> +			      fold_convert (pointer_type, BINFO_OFFSET (base)));
> +      expr = fold_convert (build_pointer_type (BINFO_TYPE (base)), expr);
> +      expr = build_fold_indirect_ref (expr);
>      }
>  
>    return expr;
> 
> I am testing the patch, but it seems like it was unfolded for a purpose.
> Mark, can you enlighten me why this expression is not folded?
> The (type)integer_cst seems particularly silly in such a common
> construct.

I'm not sure that there was any conscious decision here.  Certainly,
folding these things makes sense.  However, there is one thing to be
careful about: fold is a middle-end function, so you can't pass it
front-end trees.  But, when processing a template, there are front-end
trees around.  So, if this function is ever called while processing a
template body (like, to determine the type of an expression), then there
might be front end trees in EXPR.

That's why we have fold_if_not_in_template.  However, we don't have
fold_build2_if_not_in_template, so you can't use that approach -- unless
you want to create new functions.

I'm not sure if we can ever get to this function from within the body of
a template.  However, if you make the change you show above, you should
probably add a gcc_assert (!processing_template_decl) with a comment
explaining that if we're processing a template, we shouldn't be making
unguarded calls to the folder.  Then, if the assert trips, you may have
to create the if_not_in_template functions, or use plain
build2/build_nop when in a template, and fold_{build2,convert} otherwise.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Richard Guenther]

On 4/10/07, Jan Hubicka <jh@suse.cz> wrote:
> >
> > Hmm, maybe I can genericise the conversion for case when second operand
> > is INTEGER_CST or one of operands is NOP_EXPR that will get folded away
> > then and remove the case above.
> > I can't think of any reason why one would require checking the size
> > except for the case that someone was thinking of C pointer + operator
> > that is multiplied by the pointed to size...
> > >
> > > Otherwise this is ok if it passes testing.
> > >
> > > +           /* Convert (A+offset into &A->field_of_type_and_offset.  */
> > >
> > > parenthesis mismatch.
> >
> > Uh, I will fix it and commit without the hunk handling the conversion
> > above and will send updated patch for that.
> Hi,
> here is the updated patch, bootstrapped/regtested i686-linux, OK?
>
>         * fold-const.c (fold_unary): Convert (T1)(X op Y) into
>         ((T1)X op (T1)Y), for pointer type, when one of the new casts will fold
>         away.
> Index: fold-const.c
> ===================================================================
> *** fold-const.c        (revision 123674)
> --- fold-const.c        (working copy)
> *************** fold_unary (enum tree_code code, tree ty
> *** 7808,7831 ****
>             }
>         }
>
> !       /* Convert (T1)((T2)X op Y) into (T1)X op Y, for pointer types T1 and
> !        T2 being pointers to types of the same size.  */
> !       if (POINTER_TYPE_P (type)
>           && BINARY_CLASS_P (arg0)
> !         && TREE_CODE (TREE_OPERAND (arg0, 0)) == NOP_EXPR
> !         && POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (arg0, 0))))
>         {
>           tree arg00 = TREE_OPERAND (arg0, 0);
> !         tree t0 = type;
> !         tree t1 = TREE_TYPE (arg00);
> !         tree tt0 = TREE_TYPE (t0);
> !         tree tt1 = TREE_TYPE (t1);
> !         tree s0 = TYPE_SIZE (tt0);
> !         tree s1 = TYPE_SIZE (tt1);
> !
> !         if (s0 && s1 && operand_equal_p (s0, s1, OEP_ONLY_CONST))
> !           return build2 (TREE_CODE (arg0), t0, fold_convert (t0, arg00),
> !                          TREE_OPERAND (arg0, 1));
>         }
>
>         /* Convert (T1)(~(T2)X) into ~(T1)X if T1 and T2 are integral types
> --- 7807,7826 ----
>             }
>         }
>
> !       /* Convert (T1)(X op Y) into ((T1)X op (T1)Y), for pointer type,
> !          when one of the new casts will fold away. Conservatively we assume
> !        that this happens when X or Y is NOP_EXPR or Y is INTEGER_CST.  */
> !       if (POINTER_TYPE_P (type) && POINTER_TYPE_P (arg0)
>           && BINARY_CLASS_P (arg0)
> !         && (TREE_CODE (TREE_OPERAND (arg0, 1)) == INTEGER_CST
> !             || TREE_CODE (TREE_OPERAND (arg0, 0)) == NOP_EXPR
> !             || TREE_CODE (TREE_OPERAND (arg0, 1)) == NOP_EXPR))
>         {
>           tree arg00 = TREE_OPERAND (arg0, 0);
> !         tree arg01 = TREE_OPERAND (arg0, 1);
> !
> !         return fold_build2 (TREE_CODE (arg0), type, fold_convert (type, arg00),
> !                             fold_convert (type, arg01));
>         }

This is ok.

Thanks,
Richard.

>         /* Convert (T1)(~(T2)X) into ~(T1)X if T1 and T2 are integral types
>

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

> Jan Hubicka wrote:
> 
> > Index: gcc/cp/class.c
> > ===================================================================
> > --- gcc/cp/class.c	(revision 123674)
> > +++ gcc/cp/class.c	(working copy)
> > @@ -522,10 +522,10 @@ convert_to_base_statically (tree expr, t
> >        pointer_type = build_pointer_type (expr_type);
> >        expr = build_unary_op (ADDR_EXPR, expr, /*noconvert=*/1);
> >        if (!integer_zerop (BINFO_OFFSET (base)))
> > -	  expr = build2 (PLUS_EXPR, pointer_type, expr,
> > -			 build_nop (pointer_type, BINFO_OFFSET (base)));
> > -      expr = build_nop (build_pointer_type (BINFO_TYPE (base)), expr);
> > -      expr = build1 (INDIRECT_REF, BINFO_TYPE (base), expr);
> > +	  expr = fold_build2 (PLUS_EXPR, pointer_type, expr,
> > +			      fold_convert (pointer_type, BINFO_OFFSET (base)));
> > +      expr = fold_convert (build_pointer_type (BINFO_TYPE (base)), expr);
> > +      expr = build_fold_indirect_ref (expr);
> >      }
> >  
> >    return expr;
> > 
> > I am testing the patch, but it seems like it was unfolded for a purpose.
> > Mark, can you enlighten me why this expression is not folded?
> > The (type)integer_cst seems particularly silly in such a common
> > construct.
> 
> I'm not sure that there was any conscious decision here.  Certainly,
> folding these things makes sense.  However, there is one thing to be
> careful about: fold is a middle-end function, so you can't pass it
> front-end trees.  But, when processing a template, there are front-end
> trees around.  So, if this function is ever called while processing a
> template body (like, to determine the type of an expression), then there
> might be front end trees in EXPR.

OK, understood.  If I add fold_build2_if_not_in_template, is there
someone later who will fold the template?

Also it seems to me that instead of (type *)&base + (type *)integer_cst
we probably can go directly into the preffered form of
&base->field_representing_the_predecestor_class_in_question
in the convert_to_base_statically?
> 
> That's why we have fold_if_not_in_template.  However, we don't have
> fold_build2_if_not_in_template, so you can't use that approach -- unless
> you want to create new functions.
> 
> I'm not sure if we can ever get to this function from within the body of
> a template.  However, if you make the change you show above, you should
> probably add a gcc_assert (!processing_template_decl) with a comment
> explaining that if we're processing a template, we shouldn't be making
> unguarded calls to the folder.  Then, if the assert trips, you may have
> to create the if_not_in_template functions, or use plain
> build2/build_nop when in a template, and fold_{build2,convert} otherwise.

OK,
I will give it a try.  I already bootstrapped/regtested the patch and it
passed, so there is good chance that gcc_assert
(!processing_template_decl) will pass too.

Thanks,
Honza
> 
> -- 
> Mark Mitchell
> CodeSourcery
> mark@codesourcery.com
> (650) 331-3385 x713

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

Hi,
> I'm not sure if we can ever get to this function from within the body of
> a template.  However, if you make the change you show above, you should
> probably add a gcc_assert (!processing_template_decl) with a comment
> explaining that if we're processing a template, we shouldn't be making
> unguarded calls to the folder.  Then, if the assert trips, you may have
> to create the if_not_in_template functions, or use plain
> build2/build_nop when in a template, and fold_{build2,convert} otherwise.

I've regtested/bootstrapped this variant of patch.
Looks like we don't do that while processing_template_decls.
Does it seem OK?

	* cp/class.c (convert_to_base_statically): Fold resulting tree;
	verify that we are not processing template decl.
Index: cp/class.c
===================================================================
--- cp/class.c	(revision 123681)
+++ cp/class.c	(working copy)
@@ -531,12 +531,21 @@ convert_to_base_statically (tree expr, t
       tree pointer_type;
 
       pointer_type = build_pointer_type (expr_type);
+
+      /* fold_build2 and fold_convert can not be used on frontend trees,
+	 however this function is probably never used when processing template
+	 decls when incomplette types are present.
+
+	 When folding is disabled, this function produce trees like
+	 (type *)base + (type *)integer_cst that are left unfolded for quite
+	 long time in optimization queue.  */
+      gcc_assert (!processing_template_decl);
       expr = build_unary_op (ADDR_EXPR, expr, /*noconvert=*/1);
       if (!integer_zerop (BINFO_OFFSET (base)))
-	  expr = build2 (PLUS_EXPR, pointer_type, expr,
-			 build_nop (pointer_type, BINFO_OFFSET (base)));
-      expr = build_nop (build_pointer_type (BINFO_TYPE (base)), expr);
-      expr = build1 (INDIRECT_REF, BINFO_TYPE (base), expr);
+        expr = fold_build2 (PLUS_EXPR, pointer_type, expr,
+			    fold_convert (pointer_type, BINFO_OFFSET (base)));
+      expr = fold_convert (build_pointer_type (BINFO_TYPE (base)), expr);
+      expr = build_fold_indirect_ref (expr);
     }
 
   return expr;

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Mark Mitchell]

Jan Hubicka wrote:

> I've regtested/bootstrapped this variant of patch.
> Looks like we don't do that while processing_template_decls.
> Does it seem OK?

Yes, though I would like to copy-edit your comment. :-)

> +      /* fold_build2 and fold_convert can not be used on frontend trees,
> +	 however this function is probably never used when processing template
> +	 decls when incomplette types are present.
> +
> +	 When folding is disabled, this function produce trees like
> +	 (type *)base + (type *)integer_cst that are left unfolded for quite
> +	 long time in optimization queue.  */

/* We use fold_build2 and fold_convert below to simplify the trees
provided to the optimizers.  It is not safe to call these functions when
processing a template because they do not handle C++-specific trees.  */

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Jan Hubicka]

Hi,
unfortunately this patch had two tiny typos (mixing bits and bytes) that
disabled it in many cases.  After enabling it I've learnt that:
  1) in fold-const we should not replace type by compatible type or we drive frontends crazy.
  2) even if we disable it, the transformation into array reference is confusing frontends
     Objective C produces different protocols, C++ produces pedwarn on code like:
     char *="text" 
  3) fold_stmt is called incorrectly (fixed by separate patch)
  4) even if fold_stmt call is fixed we still ignore the fact that call to
  builtin_trap terminates control flow (I am working in fix)
  5) kernel compilation breaks on PR middle-end/31541

This patch resolve 1) and 2) by moving the simplification into gimple level
only.  First canonicalization is done during gimplification.  The benefit of
this is that transformation saves statement and for pooma testcase I have this
saves 24% of all statements.  We also don't have other place to stick it in
except in separate pass.

fold_stmt is now updated to fold away the cases introduced by inliner, CCP and
alike.

Unfortunately this does not address middle-end/31541 since gimplifier uses
langhook to mark_addressable that already sees the gimplified tree.  Moving it
out of gimplifier won't help, since we still can call back to gimplifier on
arbitrary gimple statements with some substitutions, so I think Andrew's
proposed patch to disable transformation in this case is only solution (except
for getting rid of that langhook somehow, but I don't see how)

Bootstrapped/regtested i686-linux, OK?

	* tree.h (maybe_fold_offset_to_component_ref): Remove.
	(maybe_fold_offset_to_reference): Declare.
	* fold-const.c (fold_unary): Remove maybe_fold_offset_to_component_ref
	call.
	* tree-ssa-ccp.c (maybe_fold_offset_to_array_ref): Check that size
	is known.
	(maybe_fold_offset_to_component_ref): Update docs; check that offset
	is known.
	(maybe_fold_offset_to_reference): New function.
	(maybe_fold_stmt_indirect): Use it.
	(fold_stmt_r): Fold casts of pointers.
	* gimplify.c (gimplify_conversion): Fold casts into pointers.
	(gimplify_expr): Fold addition to pointer into component reference.
Index: tree.h
===================================================================
*** tree.h	(revision 124036)
--- tree.h	(working copy)
*************** extern void fold_defer_overflow_warnings
*** 4422,4429 ****
  extern void fold_undefer_overflow_warnings (bool, tree, int);
  extern void fold_undefer_and_ignore_overflow_warnings (void);
  extern bool fold_deferring_overflow_warnings_p (void);
! extern tree maybe_fold_offset_to_component_ref (tree, tree, tree,
! 				                tree, bool);
  
  extern tree force_fit_type_double (tree, unsigned HOST_WIDE_INT, HOST_WIDE_INT,
  				   int, bool);
--- 4422,4428 ----
  extern void fold_undefer_overflow_warnings (bool, tree, int);
  extern void fold_undefer_and_ignore_overflow_warnings (void);
  extern bool fold_deferring_overflow_warnings_p (void);
! extern tree maybe_fold_offset_to_reference (tree, tree, tree);
  
  extern tree force_fit_type_double (tree, unsigned HOST_WIDE_INT, HOST_WIDE_INT,
  				   int, bool);
Index: fold-const.c
===================================================================
*** fold-const.c	(revision 124036)
--- fold-const.c	(working copy)
*************** fold_unary (enum tree_code code, tree ty
*** 7748,7760 ****
  	    return fold_convert (type, build_fold_addr_expr (base));
          }
  
-       /* Convert (type *)&A into &A->field_of_type_and_offset_0.  */
-       if (TREE_CODE (op0) == ADDR_EXPR && POINTER_TYPE_P (type)
- 	  && (tem = maybe_fold_offset_to_component_ref
- 		      (TREE_TYPE (TREE_OPERAND (op0, 0)), TREE_OPERAND (op0, 0),
- 		       integer_zero_node, TREE_TYPE (type), false)))
-         return build_fold_addr_expr_with_type (tem, type);
- 
        if ((TREE_CODE (op0) == MODIFY_EXPR
  	   || TREE_CODE (op0) == GIMPLE_MODIFY_STMT)
  	  && TREE_CONSTANT (GENERIC_TREE_OPERAND (op0, 1))
--- 7759,7764 ----
Index: tree-ssa-ccp.c
===================================================================
*** tree-ssa-ccp.c	(revision 124036)
--- tree-ssa-ccp.c	(working copy)
*************** maybe_fold_offset_to_array_ref (tree bas
*** 1584,1589 ****
--- 1584,1591 ----
       Otherwise, compute the offset as an index by using a division.  If the
       division isn't exact, then don't do anything.  */
    elt_size = TYPE_SIZE_UNIT (elt_type);
+   if (!elt_size)
+     return NULL;
    if (integer_zerop (offset))
      {
        if (TREE_CODE (elt_size) != INTEGER_CST)
*************** maybe_fold_offset_to_array_ref (tree bas
*** 1636,1647 ****
  }
  
  
! /* A subroutine of fold_stmt_r.  Attempts to fold *(S+O) to S.X.
     BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
     is the desired result type.  */
- /* ??? This doesn't handle class inheritance.  */
  
! tree
  maybe_fold_offset_to_component_ref (tree record_type, tree base, tree offset,
  				    tree orig_type, bool base_is_ptr)
  {
--- 1638,1648 ----
  }
  
  
! /* Attempt to fold *(S+O) to S.X.
     BASE is a record type.  OFFSET is a byte displacement.  ORIG_TYPE
     is the desired result type.  */
  
! static tree
  maybe_fold_offset_to_component_ref (tree record_type, tree base, tree offset,
  				    tree orig_type, bool base_is_ptr)
  {
*************** maybe_fold_offset_to_component_ref (tree
*** 1668,1673 ****
--- 1669,1676 ----
        if (DECL_BIT_FIELD (f))
  	continue;
  
+       if (!DECL_FIELD_OFFSET (f))
+ 	continue;
        field_offset = byte_position (f);
        if (TREE_CODE (field_offset) != INTEGER_CST)
  	continue;
*************** maybe_fold_offset_to_component_ref (tree
*** 1755,1760 ****
--- 1758,1826 ----
  					     orig_type, false);
  }
  
+ /* Attempt to express (ORIG_TYPE)BASE+OFFSET as BASE->field_of_orig_type
+    or BASE[index] or by combination of those. 
+ 
+    Before attempting the conversion strip off existing ADDR_EXPRs and
+    handled component refs.  */
+ 
+ tree
+ maybe_fold_offset_to_reference (tree base, tree offset, tree orig_type)
+ {
+   tree ret;
+   tree type;
+   bool base_is_ptr = true;
+ 
+   STRIP_NOPS (base);
+   if (TREE_CODE (base) == ADDR_EXPR)
+     {
+       base_is_ptr = false;
+ 
+       base = TREE_OPERAND (base, 0);
+ 
+       /* Handle case where existing COMPONENT_REF pick e.g. wrong field of union,
+ 	 so it needs to be removed and new COMPONENT_REF constructed.
+ 	 The wrong COMPONENT_REF are often constructed by folding the
+ 	 (type *)&object within the expression (type *)&object+offset  */
+       if (handled_component_p (base) && 0)
+ 	{
+           HOST_WIDE_INT sub_offset, size, maxsize;
+ 	  tree newbase;
+ 	  newbase = get_ref_base_and_extent (base, &sub_offset,
+ 					     &size, &maxsize);
+ 	  gcc_assert (newbase);
+ 	  gcc_assert (!(sub_offset & (BITS_PER_UNIT - 1)));
+ 	  if (size == maxsize)
+ 	    {
+ 	      base = newbase;
+ 	      if (sub_offset)
+ 		offset = int_const_binop (PLUS_EXPR, offset,
+ 					  build_int_cst (TREE_TYPE (offset),
+ 					  sub_offset / BITS_PER_UNIT), 1);
+ 	    }
+ 	}
+       if (lang_hooks.types_compatible_p (orig_type, TREE_TYPE (base))
+ 	  && integer_zerop (offset))
+ 	return base;
+       type = TREE_TYPE (base);
+     }
+   else
+     {
+       base_is_ptr = true;
+       if (!POINTER_TYPE_P (TREE_TYPE (base)))
+ 	return NULL_TREE;
+       type = TREE_TYPE (TREE_TYPE (base));
+     }
+   ret = maybe_fold_offset_to_component_ref (type, base, offset,
+ 					    orig_type, base_is_ptr);
+   if (!ret)
+     {
+       if (base_is_ptr)
+ 	base = build1 (INDIRECT_REF, type, base);
+       ret = maybe_fold_offset_to_array_ref (base, offset, orig_type);
+     }
+   return ret;
+ }
  
  /* A subroutine of fold_stmt_r.  Attempt to simplify *(BASE+OFFSET).
     Return the simplified expression, or NULL if nothing could be done.  */
*************** maybe_fold_stmt_indirect (tree expr, tre
*** 1791,1796 ****
--- 1857,1864 ----
  
    if (TREE_CODE (base) == ADDR_EXPR)
      {
+       tree base_addr = base;
+ 
        /* Strip the ADDR_EXPR.  */
        base = TREE_OPERAND (base, 0);
  
*************** maybe_fold_stmt_indirect (tree expr, tre
*** 1799,1822 ****
  	  && ccp_decl_initial_min_invariant (DECL_INITIAL (base)))
  	return DECL_INITIAL (base);
  
-       /* Try folding *(&B+O) to B[X].  */
-       t = maybe_fold_offset_to_array_ref (base, offset, TREE_TYPE (expr));
-       if (t)
- 	return t;
- 
        /* Try folding *(&B+O) to B.X.  */
!       t = maybe_fold_offset_to_component_ref (TREE_TYPE (base), base, offset,
! 					      TREE_TYPE (expr), false);
        if (t)
  	return t;
- 
-       /* Fold *&B to B.  We can only do this if EXPR is the same type
- 	 as BASE.  We can't do this if EXPR is the element type of an array
- 	 and BASE is the array.  */
-       if (integer_zerop (offset)
- 	  && lang_hooks.types_compatible_p (TREE_TYPE (base),
- 					    TREE_TYPE (expr)))
- 	return base;
      }
    else
      {
--- 1867,1877 ----
  	  && ccp_decl_initial_min_invariant (DECL_INITIAL (base)))
  	return DECL_INITIAL (base);
  
        /* Try folding *(&B+O) to B.X.  */
!       t = maybe_fold_offset_to_reference (base_addr, offset,
! 					  TREE_TYPE (expr));
        if (t)
  	return t;
      }
    else
      {
*************** maybe_fold_stmt_indirect (tree expr, tre
*** 1845,1853 ****
        /* Try folding *(B+O) to B->X.  Still an improvement.  */
        if (POINTER_TYPE_P (TREE_TYPE (base)))
  	{
!           t = maybe_fold_offset_to_component_ref (TREE_TYPE (TREE_TYPE (base)),
! 						  base, offset,
! 						  TREE_TYPE (expr), true);
  	  if (t)
  	    return t;
  	}
--- 1900,1907 ----
        /* Try folding *(B+O) to B->X.  Still an improvement.  */
        if (POINTER_TYPE_P (TREE_TYPE (base)))
  	{
!           t = maybe_fold_offset_to_reference (base, offset,
! 				              TREE_TYPE (expr));
  	  if (t)
  	    return t;
  	}
*************** fold_stmt_r (tree *expr_p, int *walk_sub
*** 2009,2014 ****
--- 2063,2083 ----
  				    integer_zero_node);
        break;
  
+     case NOP_EXPR:
+       t = walk_tree (&TREE_OPERAND (expr, 0), fold_stmt_r, data, NULL);
+       if (t)
+ 	return t;
+       *walk_subtrees = 0;
+ 
+       if (POINTER_TYPE_P (TREE_TYPE (expr))
+ 	  && POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (expr, 0)))
+ 	  && (t = maybe_fold_offset_to_reference
+ 		      (TREE_OPERAND (expr, 0),
+ 		       integer_zero_node,
+ 		       TREE_TYPE (TREE_TYPE (expr)))))
+         t = build_fold_addr_expr_with_type (t, TREE_TYPE (expr));
+       break;
+ 
        /* ??? Could handle more ARRAY_REFs here, as a variant of INDIRECT_REF.
  	 We'd only want to bother decomposing an existing ARRAY_REF if
  	 the base array is found to have another offset contained within.
Index: gimplify.c
===================================================================
*** gimplify.c	(revision 124036)
--- gimplify.c	(working copy)
*************** canonicalize_addr_expr (tree *expr_p)
*** 1617,1622 ****
--- 1617,1623 ----
  static enum gimplify_status
  gimplify_conversion (tree *expr_p)
  {
+   tree tem;
    gcc_assert (TREE_CODE (*expr_p) == NOP_EXPR
  	      || TREE_CODE (*expr_p) == CONVERT_EXPR);
    
*************** gimplify_conversion (tree *expr_p)
*** 1627,1632 ****
--- 1628,1644 ----
    if (tree_ssa_useless_type_conversion (*expr_p))
      *expr_p = TREE_OPERAND (*expr_p, 0);
  
+   /* Attempt to avoid NOP_EXPR by producing reference to a subtype.
+      For example this fold (subclass *)&A into &A->subclass avoiding
+      a need for statement.  */
+   if (TREE_CODE (*expr_p) == NOP_EXPR
+       && POINTER_TYPE_P (TREE_TYPE (*expr_p))
+       && POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (*expr_p, 0)))
+       && (tem = maybe_fold_offset_to_reference
+ 		  (TREE_OPERAND (*expr_p, 0),
+ 		   integer_zero_node, TREE_TYPE (TREE_TYPE (*expr_p)))))
+     *expr_p = build_fold_addr_expr_with_type (tem, TREE_TYPE (*expr_p));
+ 
    /* If we still have a conversion at the toplevel,
       then canonicalize some constructs.  */
    if (TREE_CODE (*expr_p) == NOP_EXPR || TREE_CODE (*expr_p) == CONVERT_EXPR)
*************** gimplify_expr (tree *expr_p, tree *pre_p
*** 5857,5862 ****
--- 5869,5889 ----
  	  ret = GS_ALL_DONE;
  	  break;
  
+ 	case PLUS_EXPR:
+           /* Convert ((type *)A)+offset into &A->field_of_type_and_offset.
+ 	     The second is gimple immediate saving a need for extra statement.
+ 	   */
+ 	  if (POINTER_TYPE_P (TREE_TYPE (*expr_p))
+ 	      && TREE_CODE (TREE_OPERAND (*expr_p, 1)) == INTEGER_CST
+ 	      && (tmp = maybe_fold_offset_to_reference
+ 			 (TREE_OPERAND (*expr_p, 0), TREE_OPERAND (*expr_p, 1),
+ 		   	  TREE_TYPE (TREE_TYPE (*expr_p)))))
+ 	     {
+                *expr_p = build_fold_addr_expr_with_type (tmp,
+ 							 TREE_TYPE (*expr_p));
+ 		break;
+ 	     }
+           /* FALLTHRU */
  	default:
  	  switch (TREE_CODE_CLASS (TREE_CODE (*expr_p)))
  	    {

Re: Convert (type *)&A into &A->field_of_type_and_offset_0.

[Richard Guenther]

On 4/22/07, Jan Hubicka <jh@suse.cz> wrote:
> Hi,
> unfortunately this patch had two tiny typos (mixing bits and bytes) that
> disabled it in many cases.  After enabling it I've learnt that:
>   1) in fold-const we should not replace type by compatible type or we drive frontends crazy.
>   2) even if we disable it, the transformation into array reference is confusing frontends
>      Objective C produces different protocols, C++ produces pedwarn on code like:
>      char *="text"
>   3) fold_stmt is called incorrectly (fixed by separate patch)
>   4) even if fold_stmt call is fixed we still ignore the fact that call to
>   builtin_trap terminates control flow (I am working in fix)
>   5) kernel compilation breaks on PR middle-end/31541
>
> This patch resolve 1) and 2) by moving the simplification into gimple level
> only.  First canonicalization is done during gimplification.  The benefit of
> this is that transformation saves statement and for pooma testcase I have this
> saves 24% of all statements.  We also don't have other place to stick it in
> except in separate pass.
>
> fold_stmt is now updated to fold away the cases introduced by inliner, CCP and
> alike.
>
> Unfortunately this does not address middle-end/31541 since gimplifier uses
> langhook to mark_addressable that already sees the gimplified tree.  Moving it
> out of gimplifier won't help, since we still can call back to gimplifier on
> arbitrary gimple statements with some substitutions, so I think Andrew's
> proposed patch to disable transformation in this case is only solution (except
> for getting rid of that langhook somehow, but I don't see how)
>
> Bootstrapped/regtested i686-linux, OK?

This looks good.  Please let 24 hours pass in case somebody objects
to the gimplify stuff.

Thanks,
Richard.

>         * tree.h (maybe_fold_offset_to_component_ref): Remove.
>         (maybe_fold_offset_to_reference): Declare.
>         * fold-const.c (fold_unary): Remove maybe_fold_offset_to_component_ref
>         call.
>         * tree-ssa-ccp.c (maybe_fold_offset_to_array_ref): Check that size
>         is known.
>         (maybe_fold_offset_to_component_ref): Update docs; check that offset
>         is known.
>         (maybe_fold_offset_to_reference): New function.
>         (maybe_fold_stmt_indirect): Use it.
>         (fold_stmt_r): Fold casts of pointers.
>         * gimplify.c (gimplify_conversion): Fold casts into pointers.
>         (gimplify_expr): Fold addition to pointer into component reference.