[patch] PR33600, fallout from pr33552 fix

[patch] PR33600, fallout from pr33552 fix

[Michael Matz]

Hi,

a thinko caused this.  Funny that it didn't trigger on any of the four 
test platforms, but on a kernel compile :-/

Will checkin when bootstrap and regtesting succeeds on i686.

Ciao,
Michael.
	PR inline-asm/33600
	* function.c (match_asm_constraints_1): Check for input
	being used in the outputs.

Index: gcc/function.c
===================================================================
--- gcc/function.c	(Revision 128890)
+++ gcc/function.c	(Arbeitskopie)
@@ -5716,7 +5716,7 @@ match_asm_constraints_1 (rtx insn, rtx *
 	 which wouldn't have happen without this pass.  So, iterate over
 	 all operands and replace all occurences of the register used.  */
       for (j = 0; j < noutputs; j++)
-	if (!rtx_equal_p (SET_DEST (p_sets[j]), output)
+	if (!rtx_equal_p (SET_DEST (p_sets[j]), input)
 	    && reg_overlap_mentioned_p (input, SET_DEST (p_sets[j])))
 	  SET_DEST (p_sets[j]) = replace_rtx (SET_DEST (p_sets[j]),
 					      input, output);

Re: [patch] PR33600, fallout from pr33552 fix

[Jakub Jelinek]

On Mon, Oct 01, 2007 at 04:50:59AM +0200, Michael Matz wrote:
> a thinko caused this.  Funny that it didn't trigger on any of the four 
> test platforms, but on a kernel compile :-/
> 
> Will checkin when bootstrap and regtesting succeeds on i686.

This isn't sufficient.  The optimization already in the checking phase
should bail out if there is another matching constraint pair, which uses
the same REG as we want to change.
If we have:
(insn:HI 6 3 11 2 D.c:9 (parallel [
            (set (reg/v:SI 60 [ n ])
                (asm_operands:SI ("") ("=&a") 0 [
                        (reg/v:SI 60 [ n ])
                        (reg/v:SI 60 [ n ])
                    ]
                     [
                        (asm_input:SI ("1") ("") 0)
                        (asm_input:SI ("0") ("") 0)
                    ] ("D.c") 9))
            (set (reg/v:SI 58 [ x ])
                (asm_operands:SI ("") ("=r") 1 [
                        (reg/v:SI 60 [ n ])
                        (reg/v:SI 60 [ n ])
                    ]
                     [
                        (asm_input:SI ("1") ("") 0)
                        (asm_input:SI ("0") ("") 0)
                    ] ("D.c") 9))
            (clobber (reg:QI 18 fpsr))
            (clobber (reg:QI 17 flags))
        ]) -1 (expr_list:REG_UNUSED (reg/v:SI 58 [ x ])
        (expr_list:REG_UNUSED (reg:QI 18 fpsr)
            (expr_list:REG_UNUSED (reg:QI 17 flags)
                (nil)))))
then we don't want to replace all "n" inputs to "x", because it
really is not an optimization - while it improves the situation
for one pair, it pessimizes the other pair.
Now, unlike the extremely simplistic checking this optimization
does, to find if there is another pair would need to be exact checking,
i.e. parsing all the constraints.
A far simpler option would be just to bail out if there is another
dest REG with REGNO of input.  I.e. add:
	/* Check if replacing input with output doesn't pessimize
	   another matching constraint pair.  */
	if (REG_P (input))
	  {
	    for (j = 0; j < noutputs; j++)
	      if (REG_P (SET_DEST (p_sets[j]))
		  && reg_overlap_mentioned_p (input, SET_DEST (p_sets[j])))
	        break;
	    if (j < noutputs)
	      continue;
	  }
to the checking phase.

> 	PR inline-asm/33600
> 	* function.c (match_asm_constraints_1): Check for input
> 	being used in the outputs.
> 
> Index: gcc/function.c
> ===================================================================
> --- gcc/function.c	(Revision 128890)
> +++ gcc/function.c	(Arbeitskopie)
> @@ -5716,7 +5716,7 @@ match_asm_constraints_1 (rtx insn, rtx *
>  	 which wouldn't have happen without this pass.  So, iterate over
>  	 all operands and replace all occurences of the register used.  */
>        for (j = 0; j < noutputs; j++)
> -	if (!rtx_equal_p (SET_DEST (p_sets[j]), output)
> +	if (!rtx_equal_p (SET_DEST (p_sets[j]), input)
>  	    && reg_overlap_mentioned_p (input, SET_DEST (p_sets[j])))
>  	  SET_DEST (p_sets[j]) = replace_rtx (SET_DEST (p_sets[j]),
>  					      input, output);

	Jakub

Re: [patch] PR33600, fallout from pr33552 fix

[Richard Guenther]

On 10/1/07, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> a thinko caused this.  Funny that it didn't trigger on any of the four
> test platforms, but on a kernel compile :-/
>
> Will checkin when bootstrap and regtesting succeeds on i686.

Please make sure to add a testcase.

Richard.

> Ciao,
> Michael.
>         PR inline-asm/33600
>         * function.c (match_asm_constraints_1): Check for input
>         being used in the outputs.
>
> Index: gcc/function.c
> ===================================================================
> --- gcc/function.c      (Revision 128890)
> +++ gcc/function.c      (Arbeitskopie)
> @@ -5716,7 +5716,7 @@ match_asm_constraints_1 (rtx insn, rtx *
>          which wouldn't have happen without this pass.  So, iterate over
>          all operands and replace all occurences of the register used.  */
>        for (j = 0; j < noutputs; j++)
> -       if (!rtx_equal_p (SET_DEST (p_sets[j]), output)
> +       if (!rtx_equal_p (SET_DEST (p_sets[j]), input)
>             && reg_overlap_mentioned_p (input, SET_DEST (p_sets[j])))
>           SET_DEST (p_sets[j]) = replace_rtx (SET_DEST (p_sets[j]),
>                                               input, output);
>

Re: [patch] PR33600, fallout from pr33552 fix

[Michael Matz]

Hi,

On Mon, 1 Oct 2007, Jakub Jelinek wrote:

> On Mon, Oct 01, 2007 at 04:50:59AM +0200, Michael Matz wrote:
> > a thinko caused this.  Funny that it didn't trigger on any of the four 
> > test platforms, but on a kernel compile :-/
> > 
> > Will checkin when bootstrap and regtesting succeeds on i686.
> 
> This isn't sufficient.

It is sufficient but not optimal.

> The optimization already in the checking phase
> should bail out if there is another matching constraint pair, which uses
> the same REG as we want to change.
> ...
> then we don't want to replace all "n" inputs to "x", because it
> really is not an optimization - while it improves the situation
> for one pair, it pessimizes the other pair.

I did notice the problem, but decided to not solve it for the following 
reason: The problem is harmless (in the sense that it first fixes up one 
pair, and then fixes up the other pair, i.e. emits two moves which aren't 
really necessary, as the final instruction is just as bad as the initial 
one).  And as you say really solving this would mean some much more 
complex solution, without large gain.  No large gain, because even those 
two superfluous moves will be coalesced away by the reg allocator.

I also considered some easier work-around along your lines.  The problem 
with that is that it might make this pass not do what it's supposed to do.  
If I were to not fix up one constraint pair simply because an output is 
already the same as an input we might miss fixing up a situation which 
really requires it, namely in the case that that other output did _not_ 
have a matching problematic pair.

For instance with that additional cutoff the following testcase would not 
compile anymore with -O2 -fPIC on x86 (it does with my patch):

void use (int);
void f (int a, int b, int c, int d, int e, int f)
{
  int orig = a;
  __asm__  __volatile__ (""
           : "=r" (a), "=mr" (a)
           : "0"   (a), "1" (a), "r"(c), "r"(d), "r"(e), "r"(f)  );
  use (orig);
}

So, I rather leave in the harmless short-living code pessimization which 
will be magically fixed by coalescing, instead of reducing the number of 
cases where this transformation applies.  I hold off committing the patch 
for now nevertheless to hear your opinion about the above.


Ciao,
Michael.

Re: [patch] PR33600, fallout from pr33552 fix

[Segher Boessenkool]

> a thinko caused this.  Funny that it didn't trigger on any of the four
> test platforms, but on a kernel compile :-/
>
> Will checkin when bootstrap and regtesting succeeds on i686.

Verified to fix the original problem (i386 Linux kernel compile).

Thanks for the quick fix!


Segher

Re: [patch] PR33600, fallout from pr33552 fix

[Jakub Jelinek]

On Mon, Oct 01, 2007 at 05:46:01PM +0200, Michael Matz wrote:
> So, I rather leave in the harmless short-living code pessimization which 
> will be magically fixed by coalescing, instead of reducing the number of 
> cases where this transformation applies.  I hold off committing the patch 
> for now nevertheless to hear your opinion about the above.

If you think RA's coalescing will fix this up, then why not do this
optimization through creating a new pseudo and only replacing the
input/output pair that needs it?
So for
asm ("... " : "=r" (i) : "0" (j), "r" (j));
this would add
(set (reg temp_pseudo) (reg j))
asm ("... " : "=r" (temp_pseudo) : "0" (temp_pseudo), "r" (j));
(set (reg i) (reg temp_pseudo))

	Jakub

Re: [patch] PR33600, fallout from pr33552 fix

[Michael Matz]

Hi,

On Tue, 2 Oct 2007, Jakub Jelinek wrote:

> On Mon, Oct 01, 2007 at 05:46:01PM +0200, Michael Matz wrote:
> > So, I rather leave in the harmless short-living code pessimization which 
> > will be magically fixed by coalescing, instead of reducing the number of 
> > cases where this transformation applies.  I hold off committing the patch 
> > for now nevertheless to hear your opinion about the above.
> 
> If you think RA's coalescing will fix this up, then why not do this
> optimization through creating a new pseudo and only replacing the
> input/output pair that needs it?

That was done before my patch, and lead the the non-compile of glibc.

> So for
> asm ("... " : "=r" (i) : "0" (j), "r" (j));
> this would add
> (set (reg temp_pseudo) (reg j))
> asm ("... " : "=r" (temp_pseudo) : "0" (temp_pseudo), "r" (j));
> (set (reg i) (reg temp_pseudo))

Because then we again have two inputs (temp_pseudo and j), where formerly 
we had only one.  Not to mention the additional move insn back into the 
former output.  The RA coalescing is not the goal of this pass, it just 
happens to make the possible code pessimization be harmless.


Ciao,
Michael.

Re: [patch] PR33600, fallout from pr33552 fix

[Michael Matz]

Hi,

On Tue, 2 Oct 2007, Michael Matz wrote:

> Because then we again have two inputs (temp_pseudo and j), where 
> formerly we had only one.  Not to mention the additional move insn back 
> into the former output.  The RA coalescing is not the goal of this pass, 
> it just happens to make the possible code pessimization be harmless.

I've checked this in now.


Ciao,
Michael.