Message ID | 20100608070215.GA12438@kam.mff.cuni.cz |
---|---|
State | New |
Headers | show |
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. */
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. */ >
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
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. */