Patchwork Fix overzealous DSE on sparc

login
register
mail settings
Submitter David Miller
Date May 3, 2012, 5:15 a.m.
Message ID <20120503.011527.1210866546187565505.davem@davemloft.net>
Download mbox | patch
Permalink /patch/156610/
State New
Headers show

Comments

David Miller - May 3, 2012, 5:15 a.m.
From: Richard Sandiford <rdsandiford@googlemail.com>
Date: Wed, 02 May 2012 20:37:58 +0100

> I think the DSE assuption is fair though.  If you reuse MEMs like
> this, then they're no longer just serving the purpose described by
> MEM_EXPR.

The following seems to work, and matches what calls.c does when it
passes a value by reference.  Is this what you had in mind?

2012-05-02  David S. Miller  <davem@davemloft.net>

	* config/sparc/sparc.c (emit_soft_tfmode_libcall): If we pass a
	MEM directly into a libcall, mark it's MEM_EXPR as addressable.
	(sparc_emit_float_lib_cmp): Likewise.
Richard Sandiford - May 3, 2012, 8:31 a.m.
David Miller <davem@davemloft.net> writes:
> From: Richard Sandiford <rdsandiford@googlemail.com>
> Date: Wed, 02 May 2012 20:37:58 +0100
>
>> I think the DSE assuption is fair though.  If you reuse MEMs like
>> this, then they're no longer just serving the purpose described by
>> MEM_EXPR.
>
> The following seems to work, and matches what calls.c does when it
> passes a value by reference.  Is this what you had in mind?

Yeah, looks good to me.

Richard
Richard Guenther - May 3, 2012, 8:42 a.m.
On Thu, May 3, 2012 at 10:31 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> David Miller <davem@davemloft.net> writes:
>> From: Richard Sandiford <rdsandiford@googlemail.com>
>> Date: Wed, 02 May 2012 20:37:58 +0100
>>
>>> I think the DSE assuption is fair though.  If you reuse MEMs like
>>> this, then they're no longer just serving the purpose described by
>>> MEM_EXPR.
>>
>> The following seems to work, and matches what calls.c does when it
>> passes a value by reference.  Is this what you had in mind?
>
> Yeah, looks good to me.

I don't think that will work reliably (well, maybe now by luck, so better than
nothing).  You'd at least need to adjust the ESCAPED points-to set of the
function, too (yes, DSE does very very conservative use analysis right now).
Why not simply clear MEM_EXPR for the MEM?

Richard.

> Richard
Richard Sandiford - May 3, 2012, 9:17 a.m.
Richard Guenther <richard.guenther@gmail.com> writes:
> On Thu, May 3, 2012 at 10:31 AM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> David Miller <davem@davemloft.net> writes:
>>> From: Richard Sandiford <rdsandiford@googlemail.com>
>>> Date: Wed, 02 May 2012 20:37:58 +0100
>>>
>>>> I think the DSE assuption is fair though.  If you reuse MEMs like
>>>> this, then they're no longer just serving the purpose described by
>>>> MEM_EXPR.
>>>
>>> The following seems to work, and matches what calls.c does when it
>>> passes a value by reference.  Is this what you had in mind?
>>
>> Yeah, looks good to me.
>
> I don't think that will work reliably (well, maybe now by luck, so better than
> nothing).  You'd at least need to adjust the ESCAPED points-to set of the
> function, too (yes, DSE does very very conservative use analysis right now).

Ah.

> Why not simply clear MEM_EXPR for the MEM?

The problem is that MEM rtxes aren't shared.  AIUI, the store will already
have been emitted by this point, using a distinct (but equivalent) MEM rtx.

Richard
David Miller - May 3, 2012, 9:29 a.m.
From: Richard Guenther <richard.guenther@gmail.com>
Date: Thu, 3 May 2012 10:42:30 +0200

> On Thu, May 3, 2012 at 10:31 AM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> David Miller <davem@davemloft.net> writes:
>>> From: Richard Sandiford <rdsandiford@googlemail.com>
>>> Date: Wed, 02 May 2012 20:37:58 +0100
>>>
>>>> I think the DSE assuption is fair though.  If you reuse MEMs like
>>>> this, then they're no longer just serving the purpose described by
>>>> MEM_EXPR.
>>>
>>> The following seems to work, and matches what calls.c does when it
>>> passes a value by reference.  Is this what you had in mind?
>>
>> Yeah, looks good to me.
> 
> I don't think that will work reliably (well, maybe now by luck, so better than
> nothing).  You'd at least need to adjust the ESCAPED points-to set of the
> function, too (yes, DSE does very very conservative use analysis right now).
> Why not simply clear MEM_EXPR for the MEM?

Then why does calls.c not have to do this?
David Miller - May 3, 2012, 9:38 a.m.
From: Richard Sandiford <rdsandiford@googlemail.com>
Date: Thu, 03 May 2012 10:17:44 +0100

> Richard Guenther <richard.guenther@gmail.com> writes:
>> On Thu, May 3, 2012 at 10:31 AM, Richard Sandiford
>> <rdsandiford@googlemail.com> wrote:
>>> David Miller <davem@davemloft.net> writes:
>>>> From: Richard Sandiford <rdsandiford@googlemail.com>
>>>> Date: Wed, 02 May 2012 20:37:58 +0100
>>>>
>>>>> I think the DSE assuption is fair though.  If you reuse MEMs like
>>>>> this, then they're no longer just serving the purpose described by
>>>>> MEM_EXPR.
>>>>
>>>> The following seems to work, and matches what calls.c does when it
>>>> passes a value by reference.  Is this what you had in mind?
>>>
>>> Yeah, looks good to me.
>>
>> I don't think that will work reliably (well, maybe now by luck, so better than
>> nothing).  You'd at least need to adjust the ESCAPED points-to set of the
>> function, too (yes, DSE does very very conservative use analysis right now).
> 
> Ah.
> 
>> Why not simply clear MEM_EXPR for the MEM?
> 
> The problem is that MEM rtxes aren't shared.  AIUI, the store will already
> have been emitted by this point, using a distinct (but equivalent) MEM rtx.

Again, why doesn't calls.c have to do anything about this when it
passes arguments by reference?  It does exactly what I'm changing the
sparc libcall code to do.
Richard Guenther - May 3, 2012, 9:48 a.m.
On Thu, May 3, 2012 at 11:38 AM, David Miller <davem@davemloft.net> wrote:
> From: Richard Sandiford <rdsandiford@googlemail.com>
> Date: Thu, 03 May 2012 10:17:44 +0100
>
>> Richard Guenther <richard.guenther@gmail.com> writes:
>>> On Thu, May 3, 2012 at 10:31 AM, Richard Sandiford
>>> <rdsandiford@googlemail.com> wrote:
>>>> David Miller <davem@davemloft.net> writes:
>>>>> From: Richard Sandiford <rdsandiford@googlemail.com>
>>>>> Date: Wed, 02 May 2012 20:37:58 +0100
>>>>>
>>>>>> I think the DSE assuption is fair though.  If you reuse MEMs like
>>>>>> this, then they're no longer just serving the purpose described by
>>>>>> MEM_EXPR.
>>>>>
>>>>> The following seems to work, and matches what calls.c does when it
>>>>> passes a value by reference.  Is this what you had in mind?
>>>>
>>>> Yeah, looks good to me.
>>>
>>> I don't think that will work reliably (well, maybe now by luck, so better than
>>> nothing).  You'd at least need to adjust the ESCAPED points-to set of the
>>> function, too (yes, DSE does very very conservative use analysis right now).
>>
>> Ah.
>>
>>> Why not simply clear MEM_EXPR for the MEM?
>>
>> The problem is that MEM rtxes aren't shared.  AIUI, the store will already
>> have been emitted by this point, using a distinct (but equivalent) MEM rtx.
>
> Again, why doesn't calls.c have to do anything about this when it
> passes arguments by reference?  It does exactly what I'm changing the
> sparc libcall code to do.

calls.c is unsafe, too.  Which is probably why making DSE stronger for
calls using the usual alias predicates did not work.

Richard.
David Miller - May 3, 2012, 9:52 a.m.
From: Richard Guenther <richard.guenther@gmail.com>
Date: Thu, 3 May 2012 11:48:03 +0200

> calls.c is unsafe, too.  Which is probably why making DSE stronger for
> calls using the usual alias predicates did not work.

Well, when calls.c does something more sophisticated we can do similarly
for cases like sparc's libcalls.
Richard Guenther - May 3, 2012, 10:25 a.m.
On Thu, May 3, 2012 at 11:52 AM, David Miller <davem@davemloft.net> wrote:
> From: Richard Guenther <richard.guenther@gmail.com>
> Date: Thu, 3 May 2012 11:48:03 +0200
>
>> calls.c is unsafe, too.  Which is probably why making DSE stronger for
>> calls using the usual alias predicates did not work.
>
> Well, when calls.c does something more sophisticated we can do similarly
> for cases like sparc's libcalls.

Sure.
David Miller - May 3, 2012, 5:29 p.m.
From: Richard Guenther <richard.guenther@gmail.com>
Date: Thu, 3 May 2012 12:25:11 +0200

> On Thu, May 3, 2012 at 11:52 AM, David Miller <davem@davemloft.net> wrote:
>> From: Richard Guenther <richard.guenther@gmail.com>
>> Date: Thu, 3 May 2012 11:48:03 +0200
>>
>>> calls.c is unsafe, too.  Which is probably why making DSE stronger for
>>> calls using the usual alias predicates did not work.
>>
>> Well, when calls.c does something more sophisticated we can do similarly
>> for cases like sparc's libcalls.
> 
> Sure.

Ok, so I plan to push this sparc fix into mainline and the 4.7 branch after
my testing is done.

Eric, any objections?
Eric Botcazou - May 6, 2012, 9:31 a.m.
> Ok, so I plan to push this sparc fix into mainline and the 4.7 branch after
> my testing is done.
>
> Eric, any objections?

For the record, none.

Patch

diff --git a/gcc/config/sparc/sparc.c b/gcc/config/sparc/sparc.c
index b987648..7434c0f 100644
--- a/gcc/config/sparc/sparc.c
+++ b/gcc/config/sparc/sparc.c
@@ -2698,7 +2698,12 @@  emit_soft_tfmode_libcall (const char *func_name, int nargs, rtx *operands)
 
 	  if (GET_CODE (this_arg) == MEM
 	      && ! force_stack_temp)
-	    this_arg = XEXP (this_arg, 0);
+	    {
+	      tree expr = MEM_EXPR (this_arg);
+	      if (expr)
+		mark_addressable (expr);
+	      this_arg = XEXP (this_arg, 0);
+	    }
 	  else if (CONSTANT_P (this_arg)
 		   && ! force_stack_temp)
 	    {
@@ -7387,7 +7392,12 @@  sparc_emit_float_lib_cmp (rtx x, rtx y, enum rtx_code comparison)
   if (TARGET_ARCH64)
     {
       if (MEM_P (x))
-	slot0 = x;
+	{
+	  tree expr = MEM_EXPR (x);
+	  if (expr)
+	    mark_addressable (expr);
+	  slot0 = x;
+	}
       else
 	{
 	  slot0 = assign_stack_temp (TFmode, GET_MODE_SIZE(TFmode), 0);
@@ -7395,7 +7405,12 @@  sparc_emit_float_lib_cmp (rtx x, rtx y, enum rtx_code comparison)
 	}
 
       if (MEM_P (y))
-	slot1 = y;
+	{
+	  tree expr = MEM_EXPR (y);
+	  if (expr)
+	    mark_addressable (expr);
+	  slot1 = y;
+	}
       else
 	{
 	  slot1 = assign_stack_temp (TFmode, GET_MODE_SIZE(TFmode), 0);