| Message ID | 20101129143926.GA10121@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Nov 29, 2010 at 3:39 PM, Aldy Hernandez <aldyh@redhat.com> wrote: > operand_equal_p() will return false for volatiles, even if they are exactly > the same pointer because of the side-effects. This causes problems when > we search for an existing hash slot, because we never match ourselves. > > Fixed thus. > > OK for branch? But that'll still treat s.a and s.a as different when s is volatile. Richard. > PR/46653 > * trans-mem.c (tm_log_eq): Return true for identical addresses. > > Index: testsuite/g++.dg/tm/pr46653.C > =================================================================== > --- testsuite/g++.dg/tm/pr46653.C (revision 0) > +++ testsuite/g++.dg/tm/pr46653.C (revision 0) > @@ -0,0 +1,18 @@ > +// { dg-do compile } > +// { dg-options "-fgnu-tm -O" } > + > +class shared_count > +{ > +public: > + volatile int j; > + shared_count() : j(0) { } > +}; > + > +shared_count * c; > +int main() > +{ > + __transaction [[atomic]] { > + shared_count sc; > + } > + return 0; > +} > Index: trans-mem.c > =================================================================== > --- trans-mem.c (revision 167111) > +++ trans-mem.c (working copy) > @@ -860,6 +860,13 @@ tm_log_eq (const void *p1, const void *p > { > const struct tm_log_entry *log1 = (const struct tm_log_entry *) p1; > const struct tm_log_entry *log2 = (const struct tm_log_entry *) p2; > + > + /* Special case plain equality because operand_equal_p() below will > + return FALSE if the addresses are equal but they have > + side-effects (e.g. a volatile address). */ > + if (log1->addr == log2->addr) > + return true; > + > return operand_equal_p (log1->addr, log2->addr, 0); > } > >
On 11/29/10 09:51, Richard Guenther wrote: > On Mon, Nov 29, 2010 at 3:39 PM, Aldy Hernandez<aldyh@redhat.com> wrote: > >> operand_equal_p() will return false for volatiles, even if they are exactly >> the same pointer because of the side-effects. This causes problems when >> we search for an existing hash slot, because we never match ourselves. >> >> Fixed thus. >> >> OK for branch? >> > But that'll still treat s.a and s.a as different when s is volatile. > What do you mean? The whole point of the patch is to treat them the same regardless of the volatile specifier. For that matter, I think we could get away with just using pointer equality for the TM log hash equality function (though probably not for the TM memory optimization pass).
On Mon, Nov 29, 2010 at 4:05 PM, Aldy Hernandez <aldyh@redhat.com> wrote: > On 11/29/10 09:51, Richard Guenther wrote: >> >> On Mon, Nov 29, 2010 at 3:39 PM, Aldy Hernandez<aldyh@redhat.com> wrote: >> >>> >>> operand_equal_p() will return false for volatiles, even if they are >>> exactly >>> the same pointer because of the side-effects. This causes problems when >>> we search for an existing hash slot, because we never match ourselves. >>> >>> Fixed thus. >>> >>> OK for branch? >>> >> >> But that'll still treat s.a and s.a as different when s is volatile. >> > > What do you mean? The whole point of the patch is to treat them the same > regardless of the volatile specifier. Yes, but COMPONENT_REF<s, a> isn't shared so the pointer comparison will fail. Richard.
> Yes, but COMPONENT_REF<s, a> isn't shared so the pointer comparison > will fail. > Ah, I see. What do you suggest, special casing COMPONENT_REF as well?
On Mon, Nov 29, 2010 at 4:45 PM, Aldy Hernandez <aldyh@redhat.com> wrote: > >> Yes, but COMPONENT_REF<s, a> isn't shared so the pointer comparison >> will fail. >> > > Ah, I see. What do you suggest, special casing COMPONENT_REF as well? Hm, I'm not sure. If s.a is an address then s.a is not equal to s.a. I don't understand the testcase - what would be the address you are comparing (and for what)? Richard.
On 11/29/2010 07:45 AM, Aldy Hernandez wrote: > >> Yes, but COMPONENT_REF<s, a> isn't shared so the pointer comparison >> will fail. >> > > Ah, I see. What do you suggest, special casing COMPONENT_REF as well? I suggest that we get rid of the component refs etc. I.e. resolve the reference to base + offset. We may need to actually finish a merge with mainline for this, since we'd like to be presented with Richi's MEM_REF_EXPRs more often than not. But in the meantime your tm_log_entry could save the results of get_inner_reference. r~
> I suggest that we get rid of the component refs etc. I.e. resolve the > reference to base + offset. > > We may need to actually finish a merge with mainline for this, since we'd > like to be presented with Richi's MEM_REF_EXPRs more often than not. But > in the meantime your tm_log_entry could save the results of get_inner_reference. > Can I get my patch in as is with a big FIXME, since it's a big improvement over the current brokenness? I promise to fix it soonish (next week), after I flush the big breakage in the branch. ??
On 11/29/2010 08:15 AM, Aldy Hernandez wrote: > Can I get my patch in as is with a big FIXME, since it's a big > improvement over the current brokenness? I promise to fix it soonish > (next week), after I flush the big breakage in the branch. Yeah, ok. r~
Index: testsuite/g++.dg/tm/pr46653.C =================================================================== --- testsuite/g++.dg/tm/pr46653.C (revision 0) +++ testsuite/g++.dg/tm/pr46653.C (revision 0) @@ -0,0 +1,18 @@ +// { dg-do compile } +// { dg-options "-fgnu-tm -O" } + +class shared_count +{ +public: + volatile int j; + shared_count() : j(0) { } +}; + +shared_count * c; +int main() +{ + __transaction [[atomic]] { + shared_count sc; + } + return 0; +} Index: trans-mem.c =================================================================== --- trans-mem.c (revision 167111) +++ trans-mem.c (working copy) @@ -860,6 +860,13 @@ tm_log_eq (const void *p1, const void *p { const struct tm_log_entry *log1 = (const struct tm_log_entry *) p1; const struct tm_log_entry *log2 = (const struct tm_log_entry *) p2; + + /* Special case plain equality because operand_equal_p() below will + return FALSE if the addresses are equal but they have + side-effects (e.g. a volatile address). */ + if (log1->addr == log2->addr) + return true; + return operand_equal_p (log1->addr, log2->addr, 0); }