Patchwork Fix g++.dg/torture/pr32304.C

login
register
mail settings
Submitter Jan Hubicka
Date June 8, 2010, 7:02 a.m.
Message ID <20100608070215.GA12438@kam.mff.cuni.cz>
Download mbox | patch
Permalink /patch/54937/
State New
Headers show

Comments

Jan Hubicka - June 8, 2010, 7:02 a.m.
Hi,
the testcase:

struct S {
        S() {}
};
S f() {
        static S s;
        return s;
}

fails with my last ipa-pure-const changes because variable S that is
TYPE_NEEDS_CONSTRCTING become read only.  Old pure-const implementation never
did this change because TYPE_NEEDS_CONSTRUCTIONG vars was left out of analysis.

I am not too familiar with TYPE_NEEDS_CONSTRUCTING mechanism, but I think in
this case S can become read only.  The reason is that S is empty and there is
nothing to construct.  We compile the code into:
S f() ()
{ 
  struct S D.2138;
  bool D.2122;
  int D.2130;
  bool retval.1;
  char D.2126;
  char * _ZGVZ1fvE1s.0;

<bb 2>:
  _ZGVZ1fvE1s.0_1 = (char *) &_ZGVZ1fvE1s;
  D.2126_2 = *_ZGVZ1fvE1s.0_1;
  if (D.2126_2 == 0)
    goto <bb 3>;
  else
    goto <bb 5>;

<bb 3>:
  D.2130_3 = __cxa_guard_acquire (&_ZGVZ1fvE1s);
  if (D.2130_3 != 0)
    goto <bb 4>;
  else
    goto <bb 5>;

<bb 4>:
  __cxa_guard_release (&_ZGVZ1fvE1s);

<bb 5>:
  return D.2138;

}

where __cxa_guard_acquire/__cxa_guard_release pair is locking the empty
initialization code, at least in my understanding.  Looking at places
TYPE_NEEDS_CONSTRUCTING is handled in backend, I think we should have
everything gimplified to level that we do not need to care about the flag.

Note also that D.2138 is undefined. It is comming so from frontend while in
older versions we at least used to initialize it from static s.  I wonder if
this is because the var is empty.

Does the attached patch seem to make sense?

Bootstrapped/regtested x86_64-linux.

	* emit-rtl.c (set_mem_attributes_minus_bitpos): Remove TYPE_NEEDS_CONSTRUCTING
	sanity check.
Andrew Pinski - June 8, 2010, 5:10 p.m.
Sent from my iPhone

On Jun 8, 2010, at 12:02 AM, Jan Hubicka <hubicka@ucw.cz> wrote:

> Hi,
> the testcase:
>
> struct S {
>        S() {}
> };
> S f() {
>        static S s;
>        return s;
> }
>
> fails with my last ipa-pure-const changes because variable S that is
> TYPE_NEEDS_CONSTRCTING become read only.  Old pure-const  
> implementation never
> did this change because TYPE_NEEDS_CONSTRUCTIONG vars was left out  
> of analysis.
>
> I am not too familiar with TYPE_NEEDS_CONSTRUCTING mechanism, but I  
> think in
> this case S can become read only.  The reason is that S is empty and  
> there is
> nothing to construct.  We compile the code into:
> S f() ()
> {
>  struct S D.2138;
>  bool D.2122;
>  int D.2130;
>  bool retval.1;
>  char D.2126;
>  char * _ZGVZ1fvE1s.0;
>
> <bb 2>:
>  _ZGVZ1fvE1s.0_1 = (char *) &_ZGVZ1fvE1s;
>  D.2126_2 = *_ZGVZ1fvE1s.0_1;
>  if (D.2126_2 == 0)
>    goto <bb 3>;
>  else
>    goto <bb 5>;
>
> <bb 3>:
>  D.2130_3 = __cxa_guard_acquire (&_ZGVZ1fvE1s);
>  if (D.2130_3 != 0)
>    goto <bb 4>;
>  else
>    goto <bb 5>;
>
> <bb 4>:
>  __cxa_guard_release (&_ZGVZ1fvE1s);
>
> <bb 5>:
>  return D.2138;
>
> }
>
> where __cxa_guard_acquire/__cxa_guard_release pair is locking the  
> empty
> initialization code, at least in my understanding.  Looking at places
> TYPE_NEEDS_CONSTRUCTING is handled in backend, I think we should have
> everything gimplified to level that we do not need to care about the  
> flag.
>
> Note also that D.2138 is undefined. It is comming so from frontend  
> while in
> older versions we at least used to initialize it from static s.  I  
> wonder if
> this is because the var is empty.

Yes it is because its type is empty though padded to size of one.

>
> Does the attached patch seem to make sense?
>
> Bootstrapped/regtested x86_64-linux.
>
>    * emit-rtl.c (set_mem_attributes_minus_bitpos): Remove  
> TYPE_NEEDS_CONSTRUCTING
>    sanity check.
> Index: emit-rtl.c
> ===================================================================
> --- emit-rtl.c    (revision 160379)
> +++ emit-rtl.c    (working copy)
> @@ -1668,12 +1668,7 @@ set_mem_attributes_minus_bitpos (rtx ref
>       if (base && DECL_P (base)
>      && TREE_READONLY (base)
>      && (TREE_STATIC (base) || DECL_EXTERNAL (base)))
> -    {
> -      tree base_type = TREE_TYPE (base);
> -      gcc_assert (!(base_type && TYPE_NEEDS_CONSTRUCTING (base_type))
> -              || DECL_ARTIFICIAL (base));
> -      MEM_READONLY_P (ref) = 1;
> -    }
> +    MEM_READONLY_P (ref) = 1;
>
>       /* If this expression uses it's parent's alias set, mark it such
>     that we won't change it.  */
Richard Guenther - June 10, 2010, 9:13 a.m.
On Tue, Jun 8, 2010 at 9:02 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> the testcase:
>
> struct S {
>        S() {}
> };
> S f() {
>        static S s;
>        return s;
> }
>
> fails with my last ipa-pure-const changes because variable S that is
> TYPE_NEEDS_CONSTRCTING become read only.  Old pure-const implementation never
> did this change because TYPE_NEEDS_CONSTRUCTIONG vars was left out of analysis.
>
> I am not too familiar with TYPE_NEEDS_CONSTRUCTING mechanism, but I think in
> this case S can become read only.  The reason is that S is empty and there is
> nothing to construct.  We compile the code into:
> S f() ()
> {
>  struct S D.2138;
>  bool D.2122;
>  int D.2130;
>  bool retval.1;
>  char D.2126;
>  char * _ZGVZ1fvE1s.0;
>
> <bb 2>:
>  _ZGVZ1fvE1s.0_1 = (char *) &_ZGVZ1fvE1s;
>  D.2126_2 = *_ZGVZ1fvE1s.0_1;
>  if (D.2126_2 == 0)
>    goto <bb 3>;
>  else
>    goto <bb 5>;
>
> <bb 3>:
>  D.2130_3 = __cxa_guard_acquire (&_ZGVZ1fvE1s);
>  if (D.2130_3 != 0)
>    goto <bb 4>;
>  else
>    goto <bb 5>;
>
> <bb 4>:
>  __cxa_guard_release (&_ZGVZ1fvE1s);
>
> <bb 5>:
>  return D.2138;
>
> }
>
> where __cxa_guard_acquire/__cxa_guard_release pair is locking the empty
> initialization code, at least in my understanding.  Looking at places
> TYPE_NEEDS_CONSTRUCTING is handled in backend, I think we should have
> everything gimplified to level that we do not need to care about the flag.
>
> Note also that D.2138 is undefined. It is comming so from frontend while in
> older versions we at least used to initialize it from static s.  I wonder if
> this is because the var is empty.
>
> Does the attached patch seem to make sense?

I guess so.

Ok.

Thanks,
Richard.

> Bootstrapped/regtested x86_64-linux.
>
>        * emit-rtl.c (set_mem_attributes_minus_bitpos): Remove TYPE_NEEDS_CONSTRUCTING
>        sanity check.
> Index: emit-rtl.c
> ===================================================================
> --- emit-rtl.c  (revision 160379)
> +++ emit-rtl.c  (working copy)
> @@ -1668,12 +1668,7 @@ set_mem_attributes_minus_bitpos (rtx ref
>       if (base && DECL_P (base)
>          && TREE_READONLY (base)
>          && (TREE_STATIC (base) || DECL_EXTERNAL (base)))
> -       {
> -         tree base_type = TREE_TYPE (base);
> -         gcc_assert (!(base_type && TYPE_NEEDS_CONSTRUCTING (base_type))
> -                     || DECL_ARTIFICIAL (base));
> -         MEM_READONLY_P (ref) = 1;
> -       }
> +       MEM_READONLY_P (ref) = 1;
>
>       /* If this expression uses it's parent's alias set, mark it such
>         that we won't change it.  */
>
Hans-Peter Nilsson - June 10, 2010, 10:42 a.m.
On Thu, 10 Jun 2010, Richard Guenther wrote:
> On Tue, Jun 8, 2010 at 9:02 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> > Does the attached patch seem to make sense?
>
> I guess so.
>
> Ok.
>
> Thanks,
> Richard.
>
> > Bootstrapped/regtested x86_64-linux.
> >
> >        * emit-rtl.c (set_mem_attributes_minus_bitpos): Remove TYPE_NEEDS_CONSTRUCTING
> >        sanity check.

Committed on behalf of Honza.

brgds, H-P

Patch

Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 160379)
+++ emit-rtl.c	(working copy)
@@ -1668,12 +1668,7 @@  set_mem_attributes_minus_bitpos (rtx ref
       if (base && DECL_P (base)
 	  && TREE_READONLY (base)
 	  && (TREE_STATIC (base) || DECL_EXTERNAL (base)))
-	{
-	  tree base_type = TREE_TYPE (base);
-	  gcc_assert (!(base_type && TYPE_NEEDS_CONSTRUCTING (base_type))
-		      || DECL_ARTIFICIAL (base));
-	  MEM_READONLY_P (ref) = 1;
-	}
+	MEM_READONLY_P (ref) = 1;
 
       /* If this expression uses it's parent's alias set, mark it such
 	 that we won't change it.  */