| 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. */
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.