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

Submitted by Aldy Hernandez on Nov. 29, 2010, 2:39 p.m.

Details

Message ID 20101129143926.GA10121@redhat.com
State New
Headers show

Commit Message

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.

Comments

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 hide | download patch | download mbox

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