diff mbox

[trans-mem] handle invisible references for decls (PR/47554)

Message ID 4D4C5CC2.2060205@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Feb. 4, 2011, 8:08 p.m. UTC 
As discussed on IRC.

A RESULT_DECL that is NOT is_gimple_reg(retval), ends up in a register 
because it is an invisble reference (DECL_BY_REFERENCE).  So we should 
pass on its instrumentation, because later we'll see the corresponding 
INDIRECT_REF which we can actually instrument.

Confusing?  Yes.  Frustrating?  It's GCC, of course.  Problem fixed?  Yes!

OK for branch?
PR/47554
	* trans-mem.c (requires_barrier): Do not instrument if
	DECL_BY_REFERENCE.

Comments

Richard Henderson Feb. 4, 2011, 8:29 p.m. UTC | #1 
On 02/04/2011 12:08 PM, Aldy Hernandez wrote:
> +      if (DECL_BY_REFERENCE (x))
> +	{
> +	  /* Stupid GCC magic.  In assign_params() we'll see the
> +	     DECL_BY_REFERENCE and put the whole thing in a register,
> +	     so we can't instrument this by calculating &<retval>.  We
> +	     will see the resulting INDIRECT_REF elsewhere and
> +	     instrument that.  See... this is why GCC hackers will
> +	     always have a job, cause nobody understands this and
> +	     takes too long to rewrite it.  */
> +	  return false;

Ha ha.

A better comment might be

  /* ??? This value is a pointer, but aggregate_value_p has been
     jigged to return true which confuses needs_to_live_in_memory.
     This ought to be cleaned up generically.  */

Ok with that change.


I sincerely believe that the test in aggregate_value_p is a mistake.
We should not be lying about the fact that this value is a pointer.
But looking at how DECL_BY_REFERENCE is used elsewhere, it'll be a
hard one to unravel.  And that certainly should not be done on this
branch.


r~
Aldy Hernandez Feb. 4, 2011, 9:08 p.m. UTC | #2 
On 02/04/11 14:29, Richard Henderson wrote:
> On 02/04/2011 12:08 PM, Aldy Hernandez wrote:
>> +      if (DECL_BY_REFERENCE (x))
>> +	{
>> +	  /* Stupid GCC magic.  In assign_params() we'll see the
>> +	     DECL_BY_REFERENCE and put the whole thing in a register,
>> +	     so we can't instrument this by calculating&<retval>.  We
>> +	     will see the resulting INDIRECT_REF elsewhere and
>> +	     instrument that.  See... this is why GCC hackers will
>> +	     always have a job, cause nobody understands this and
>> +	     takes too long to rewrite it.  */
>> +	  return false;
>
> Ha ha.
>
> A better comment might be
>
>    /* ??? This value is a pointer, but aggregate_value_p has been
>       jigged to return true which confuses needs_to_live_in_memory.
>       This ought to be cleaned up generically.  */

Well, I was going to commit it as obvious to keep you from vetoing my 
comment and forcing into the sources, but I changed my mind at the last 
minute :).

> I sincerely believe that the test in aggregate_value_p is a mistake.
> We should not be lying about the fact that this value is a pointer.
> But looking at how DECL_BY_REFERENCE is used elsewhere, it'll be a
> hard one to unravel.  And that certainly should not be done on this
> branch.

La la la, can't hear you.

Aldy
Richard Biener Feb. 4, 2011, 11:26 p.m. UTC | #3 
On Fri, Feb 4, 2011 at 9:29 PM, Richard Henderson <rth@redhat.com> wrote:
> On 02/04/2011 12:08 PM, Aldy Hernandez wrote:
>> +      if (DECL_BY_REFERENCE (x))
>> +     {
>> +       /* Stupid GCC magic.  In assign_params() we'll see the
>> +          DECL_BY_REFERENCE and put the whole thing in a register,
>> +          so we can't instrument this by calculating &<retval>.  We
>> +          will see the resulting INDIRECT_REF elsewhere and
>> +          instrument that.  See... this is why GCC hackers will
>> +          always have a job, cause nobody understands this and
>> +          takes too long to rewrite it.  */
>> +       return false;
>
> Ha ha.
>
> A better comment might be
>
>  /* ??? This value is a pointer, but aggregate_value_p has been
>     jigged to return true which confuses needs_to_live_in_memory.
>     This ought to be cleaned up generically.  */
>
> Ok with that change.
>
>
> I sincerely believe that the test in aggregate_value_p is a mistake.
> We should not be lying about the fact that this value is a pointer.
> But looking at how DECL_BY_REFERENCE is used elsewhere, it'll be a
> hard one to unravel.  And that certainly should not be done on this
> branch.

Hm, I think we fixed this on trunk and DECL_BY_REFERENCE result-decls
are now even in SSA form.

Richard.

>
> r~
>
diff mbox

Patch

Index: testsuite/g++.dg/tm/pr47554.C
===================================================================
--- testsuite/g++.dg/tm/pr47554.C	(revision 0)
+++ testsuite/g++.dg/tm/pr47554.C	(revision 0)
@@ -0,0 +1,27 @@ 
+// { dg-do compile }
+// { dg-options "-fgnu-tm" }
+
+class list
+{
+	public:     list()
+	{
+	}
+	list(const list&)
+	{
+	}
+	const list&       _M_get_Node_allocator() const
+	{
+	}
+	list       _M_get_Tp_allocator() const
+	{
+		return list(_M_get_Node_allocator());
+	}
+};
+static list buildProjects;
+static void build()
+{
+	__transaction [[relaxed]]
+	{
+		buildProjects._M_get_Tp_allocator();
+	}
+}
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 169830)
+++ trans-mem.c	(working copy)
@@ -1467,6 +1467,18 @@  requires_barrier (basic_block entry_bloc
     case PARM_DECL:
     case RESULT_DECL:
     case VAR_DECL:
+      if (DECL_BY_REFERENCE (x))
+	{
+	  /* Stupid GCC magic.  In assign_params() we'll see the
+	     DECL_BY_REFERENCE and put the whole thing in a register,
+	     so we can't instrument this by calculating &<retval>.  We
+	     will see the resulting INDIRECT_REF elsewhere and
+	     instrument that.  See... this is why GCC hackers will
+	     always have a job, cause nobody understands this and
+	     takes too long to rewrite it.  */
+	  return false;
+	}
+
       if (is_global_var (x))
 	return !TREE_READONLY (x);
       if (/* FIXME: This condition should actually go below in the