Patchwork [trans-mem] PR46653: handle identical log entries in hash

login
register
mail settings
Submitter Aldy Hernandez
Date Nov. 29, 2010, 2:39 p.m.
Message ID <20101129143926.GA10121@redhat.com>
Download mbox | patch
Permalink /patch/73429/
State New
Headers show

Comments

Aldy Hernandez - Nov. 29, 2010, 2:39 p.m.
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?

	PR/46653
	* trans-mem.c (tm_log_eq): Return true for identical addresses.
Richard Guenther - Nov. 29, 2010, 2:51 p.m.
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);
>  }
>
>
Aldy Hernandez - Nov. 29, 2010, 3:05 p.m.
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).
Richard Guenther - Nov. 29, 2010, 3:43 p.m.
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.
Aldy Hernandez - Nov. 29, 2010, 3:45 p.m.
> 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?
Richard Guenther - Nov. 29, 2010, 3:55 p.m.
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.
Richard Henderson - Nov. 29, 2010, 4:08 p.m.
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~
Aldy Hernandez - Nov. 29, 2010, 4:15 p.m.
> 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.

??
Richard Henderson - Nov. 29, 2010, 4:15 p.m.
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~

Patch

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);
 }