Patchwork [PR,46349] One more SRA test for aggregate bit-fields

login
register
mail settings
Submitter Martin Jambor
Date Nov. 10, 2010, 12:22 p.m.
Message ID <20101110122253.GA9941@virgil.arch.suse.de>
Download mbox | patch
Permalink /patch/70627/
State New
Headers show

Comments

Martin Jambor - Nov. 10, 2010, 12:22 p.m.
Hi,

the following is necessary to fix PR 46349.  It basically turns off a
piece of code which is there to try to get rid of VIEW_CONVERT_EXPRs
when there are aggregate bit-fields involved in the assignment.

I have gone through all other calls to build_ref_for_offset and its
callers and came to the conclusion that in most cases, tests for
contains_view_convert_expr_p should be accompanied with a test for a
bit-field component_ref because there might be assignments in between
a candidate and a union with aggregate bit-fields which might cause
the same problems and should therefore be detected.  So I added that
to the patch, even though it is not exactly pretty.  (No, I do not
have any testcases.)

Bootstrapped and tested on x86_64-linux on top of
http://gcc.gnu.org/ml/gcc-patches/2010-11/msg00933.html without any
regressions (the peculiar acats one is gone too).  OK for trunk?

Thanks,

Martin


2010-11-09  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/46349
	* tree-sra.c (contains_bitfld_comp_ref_p): New function.
	(contains_vce_or_bfcref_p): Likewise.
	(sra_modify_assign): Use them.

	* testsuite/gnat.dg/opt9.adb: New file.
	* testsuite/gnat.dg/opt9_pkg.ads: Likewise
Richard Guenther - Nov. 15, 2010, 12:20 p.m.
On Wed, Nov 10, 2010 at 1:22 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> the following is necessary to fix PR 46349.  It basically turns off a
> piece of code which is there to try to get rid of VIEW_CONVERT_EXPRs
> when there are aggregate bit-fields involved in the assignment.
>
> I have gone through all other calls to build_ref_for_offset and its
> callers and came to the conclusion that in most cases, tests for
> contains_view_convert_expr_p should be accompanied with a test for a
> bit-field component_ref because there might be assignments in between
> a candidate and a union with aggregate bit-fields which might cause
> the same problems and should therefore be detected.  So I added that
> to the patch, even though it is not exactly pretty.  (No, I do not
> have any testcases.)
>
> Bootstrapped and tested on x86_64-linux on top of
> http://gcc.gnu.org/ml/gcc-patches/2010-11/msg00933.html without any
> regressions (the peculiar acats one is gone too).  OK for trunk?

Ok.

Thanks,
Richard.

> Thanks,
>
> Martin
>
>
> 2010-11-09  Martin Jambor  <mjambor@suse.cz>
>
>        PR tree-optimization/46349
>        * tree-sra.c (contains_bitfld_comp_ref_p): New function.
>        (contains_vce_or_bfcref_p): Likewise.
>        (sra_modify_assign): Use them.
>
>        * testsuite/gnat.dg/opt9.adb: New file.
>        * testsuite/gnat.dg/opt9_pkg.ads: Likewise
>
>
> Index: mine/gcc/tree-sra.c
> ===================================================================
> --- mine.orig/gcc/tree-sra.c
> +++ mine/gcc/tree-sra.c
> @@ -2629,6 +2629,41 @@ get_repl_default_def_ssa_name (struct ac
>   return repl;
>  }
>
> +/* Return true if REF has a COMPONENT_REF with a bit-field field declaration
> +   somewhere in it.  */
> +
> +static inline bool
> +contains_bitfld_comp_ref_p (const_tree ref)
> +{
> +  while (handled_component_p (ref))
> +    {
> +      if (TREE_CODE (ref) == COMPONENT_REF
> +          && DECL_BIT_FIELD (TREE_OPERAND (ref, 1)))
> +        return true;
> +      ref = TREE_OPERAND (ref, 0);
> +    }
> +
> +  return false;
> +}
> +
> +/* Return true if REF has an VIEW_CONVERT_EXPR or a COMPONENT_REF with a
> +   bit-field field declaration somewhere in it.  */
> +
> +static inline bool
> +contains_vce_or_bfcref_p (const_tree ref)
> +{
> +  while (handled_component_p (ref))
> +    {
> +      if (TREE_CODE (ref) == VIEW_CONVERT_EXPR
> +         || (TREE_CODE (ref) == COMPONENT_REF
> +             && DECL_BIT_FIELD (TREE_OPERAND (ref, 1))))
> +       return true;
> +      ref = TREE_OPERAND (ref, 0);
> +    }
> +
> +  return false;
> +}
> +
>  /* Examine both sides of the assignment statement pointed to by STMT, replace
>    them with a scalare replacement if there is one and generate copying of
>    replacements if scalarized aggregates have been used in the assignment.  GSI
> @@ -2697,6 +2732,7 @@ sra_modify_assign (gimple *stmt, gimple_
>             ???  This should move to fold_stmt which we simply should
>             call after building a VIEW_CONVERT_EXPR here.  */
>          if (AGGREGATE_TYPE_P (TREE_TYPE (lhs))
> +             && !contains_bitfld_comp_ref_p (lhs)
>              && !access_has_children_p (lacc))
>            {
>              lhs = build_ref_for_offset (loc, lhs, 0, TREE_TYPE (rhs),
> @@ -2704,7 +2740,7 @@ sra_modify_assign (gimple *stmt, gimple_
>              gimple_assign_set_lhs (*stmt, lhs);
>            }
>          else if (AGGREGATE_TYPE_P (TREE_TYPE (rhs))
> -                  && !contains_view_convert_expr_p (rhs)
> +                  && !contains_vce_or_bfcref_p (rhs)
>                   && !access_has_children_p (racc))
>            rhs = build_ref_for_offset (loc, rhs, 0, TREE_TYPE (lhs),
>                                        gsi, false);
> @@ -2754,8 +2790,8 @@ sra_modify_assign (gimple *stmt, gimple_
>      This is what the first branch does.  */
>
>   if (gimple_has_volatile_ops (*stmt)
> -      || contains_view_convert_expr_p (rhs)
> -      || contains_view_convert_expr_p (lhs))
> +      || contains_vce_or_bfcref_p (rhs)
> +      || contains_vce_or_bfcref_p (lhs))
>     {
>       if (access_has_children_p (racc))
>        generate_subtree_copies (racc->first_child, racc->base, 0, 0, 0,
> Index: mine/gcc/testsuite/gnat.dg/opt9.adb
> ===================================================================
> --- /dev/null
> +++ mine/gcc/testsuite/gnat.dg/opt9.adb
> @@ -0,0 +1,26 @@
> +-- { dg-do compile }
> +-- { dg-options "-gnatws -O" }
> +
> +with Opt9_Pkg; use Opt9_Pkg;
> +
> +procedure Opt9 is
> +
> +   type Array_T is array (1 .. N) of Integer;
> +
> +   type Clock_T is record
> +      N_Ticks : Integer := 0;
> +   end record;
> +
> +   type Rec is record
> +      Values : Array_T;
> +      Valid  : Boolean;
> +      Tstamp : Clock_T;
> +   end record;
> +
> +   pragma Pack (Rec);
> +
> +   Data : Rec;
> +
> +begin
> +   null;
> +end;
> Index: mine/gcc/testsuite/gnat.dg/opt9_pkg.ads
> ===================================================================
> --- /dev/null
> +++ mine/gcc/testsuite/gnat.dg/opt9_pkg.ads
> @@ -0,0 +1,5 @@
> +package Opt9_Pkg is
> +
> +  N : integer := 15;
> +
> +end Opt9_Pkg;
>

Patch

Index: mine/gcc/tree-sra.c
===================================================================
--- mine.orig/gcc/tree-sra.c
+++ mine/gcc/tree-sra.c
@@ -2629,6 +2629,41 @@  get_repl_default_def_ssa_name (struct ac
   return repl;
 }
 
+/* Return true if REF has a COMPONENT_REF with a bit-field field declaration
+   somewhere in it.  */
+
+static inline bool
+contains_bitfld_comp_ref_p (const_tree ref)
+{
+  while (handled_component_p (ref))
+    {
+      if (TREE_CODE (ref) == COMPONENT_REF
+          && DECL_BIT_FIELD (TREE_OPERAND (ref, 1)))
+        return true;
+      ref = TREE_OPERAND (ref, 0);
+    }
+
+  return false;
+}
+
+/* Return true if REF has an VIEW_CONVERT_EXPR or a COMPONENT_REF with a
+   bit-field field declaration somewhere in it.  */
+
+static inline bool
+contains_vce_or_bfcref_p (const_tree ref)
+{
+  while (handled_component_p (ref))
+    {
+      if (TREE_CODE (ref) == VIEW_CONVERT_EXPR
+	  || (TREE_CODE (ref) == COMPONENT_REF
+	      && DECL_BIT_FIELD (TREE_OPERAND (ref, 1))))
+	return true;
+      ref = TREE_OPERAND (ref, 0);
+    }
+
+  return false;
+}
+
 /* Examine both sides of the assignment statement pointed to by STMT, replace
    them with a scalare replacement if there is one and generate copying of
    replacements if scalarized aggregates have been used in the assignment.  GSI
@@ -2697,6 +2732,7 @@  sra_modify_assign (gimple *stmt, gimple_
 	     ???  This should move to fold_stmt which we simply should
 	     call after building a VIEW_CONVERT_EXPR here.  */
 	  if (AGGREGATE_TYPE_P (TREE_TYPE (lhs))
+	      && !contains_bitfld_comp_ref_p (lhs)
 	      && !access_has_children_p (lacc))
 	    {
 	      lhs = build_ref_for_offset (loc, lhs, 0, TREE_TYPE (rhs),
@@ -2704,7 +2740,7 @@  sra_modify_assign (gimple *stmt, gimple_
 	      gimple_assign_set_lhs (*stmt, lhs);
 	    }
 	  else if (AGGREGATE_TYPE_P (TREE_TYPE (rhs))
-		   && !contains_view_convert_expr_p (rhs)
+		   && !contains_vce_or_bfcref_p (rhs)
 		   && !access_has_children_p (racc))
 	    rhs = build_ref_for_offset (loc, rhs, 0, TREE_TYPE (lhs),
 					gsi, false);
@@ -2754,8 +2790,8 @@  sra_modify_assign (gimple *stmt, gimple_
      This is what the first branch does.  */
 
   if (gimple_has_volatile_ops (*stmt)
-      || contains_view_convert_expr_p (rhs)
-      || contains_view_convert_expr_p (lhs))
+      || contains_vce_or_bfcref_p (rhs)
+      || contains_vce_or_bfcref_p (lhs))
     {
       if (access_has_children_p (racc))
 	generate_subtree_copies (racc->first_child, racc->base, 0, 0, 0,
Index: mine/gcc/testsuite/gnat.dg/opt9.adb
===================================================================
--- /dev/null
+++ mine/gcc/testsuite/gnat.dg/opt9.adb
@@ -0,0 +1,26 @@ 
+-- { dg-do compile }
+-- { dg-options "-gnatws -O" }
+
+with Opt9_Pkg; use Opt9_Pkg;
+
+procedure Opt9 is
+
+   type Array_T is array (1 .. N) of Integer;
+
+   type Clock_T is record
+      N_Ticks : Integer := 0;
+   end record;
+
+   type Rec is record
+      Values : Array_T;
+      Valid  : Boolean;
+      Tstamp : Clock_T;
+   end record;
+
+   pragma Pack (Rec);
+
+   Data : Rec;
+
+begin
+   null;
+end;
Index: mine/gcc/testsuite/gnat.dg/opt9_pkg.ads
===================================================================
--- /dev/null
+++ mine/gcc/testsuite/gnat.dg/opt9_pkg.ads
@@ -0,0 +1,5 @@ 
+package Opt9_Pkg is
+
+  N : integer := 15;
+
+end Opt9_Pkg;