[PATCH] recog.c: Fix RTX unsharing in change groups

[PATCH] recog.c: Fix RTX unsharing in change groups

[Andreas Krebbel]

Hi,

we currently miss to do RTX unsharing in some cases when mixing
validate_change requests with and without rtx unsharing in the same
change group.

Debugging a setjmp testcase on s390 I've seen the following behaviour:

In an insn like A = B + C B is replaced with D in fold_rtx. Unsharing is
requested for D in this replacement (A = D + C). Later on fold_rtx
invokes canonicalize_change_group which decides to swap D and C
without requesting unsharing again (A = C + D). Since the unsharing
for a change group is delayed until confirm_change_group and is bound
to a location rather then the expression C is unshared instead of D
then.

The attached patch prevents that by doing RTX unsharing for all
subsequent changes after finding a single change where unsharing has
been explicitly requested.

Ok for mainline?

Bye,

-Andreas-


2011-03-04  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>

	* recog.c (confirm_change_group): Unshare all RTXs after finding
	one in the group where unsharing has been requested.


Index: gcc/recog.c
===================================================================
*** gcc/recog.c.orig
--- gcc/recog.c
*************** confirm_change_group (void)
*** 454,465 ****
  {
    int i;
    rtx last_object = NULL;
  
    for (i = 0; i < num_changes; i++)
      {
        rtx object = changes[i].object;
  
!       if (changes[i].unshare)
  	*changes[i].loc = copy_rtx (*changes[i].loc);
  
        /* Avoid unnecessary rescanning when multiple changes to same instruction
--- 454,471 ----
  {
    int i;
    rtx last_object = NULL;
+   bool unshare_p = false;
  
    for (i = 0; i < num_changes; i++)
      {
        rtx object = changes[i].object;
  
!       /* After doing one change which needs unsharing all subsequent
! 	 changes need unsharing as well.  A subsequent change might
! 	 move parts of the object which have been requested to get
! 	 unshared before without requesting unsharing itself.  */
!       unshare_p = unshare_p || changes[i].unshare;
!       if (unshare_p)
  	*changes[i].loc = copy_rtx (*changes[i].loc);
  
        /* Avoid unnecessary rescanning when multiple changes to same instruction

Re: [PATCH] recog.c: Fix RTX unsharing in change groups

[Eric Botcazou]

> The attached patch prevents that by doing RTX unsharing for all
> subsequent changes after finding a single change where unsharing has
> been explicitly requested.

This looks like a big hammer.  Why not doing it in canonicalize_change_group, 
i.e. calling validate_unshare_change instead of validate_change there?

-- 
Eric Botcazou

Re: [PATCH] recog.c: Fix RTX unsharing in change groups

[Andreas Krebbel]

On 03/18/2011 09:11 PM, Eric Botcazou wrote:
>> The attached patch prevents that by doing RTX unsharing for all
>> subsequent changes after finding a single change where unsharing has
>> been explicitly requested.
> 
> This looks like a big hammer.  Why not doing it in canonicalize_change_group, 
> i.e. calling validate_unshare_change instead of validate_change there?

I think with the patch validate_(unshare)_change becomes easier to use. You can still
decide locally if you want unsharing or not. Without taking into account in which contexts
your function gets called. I consider this a big advantage.

Otherwise we would have to look at every validate_change invocation in order to check if
it might get called after validate_unshare_change. This sounds tedious and error-prone to
me. (and probably we would end up using validate_unshare_change everywhere)

-Andreas-

Re: [PATCH] recog.c: Fix RTX unsharing in change groups

[Eric Botcazou]

> I think with the patch validate_(unshare)_change becomes easier to use. You
> can still decide locally if you want unsharing or not. Without taking into
> account in which contexts your function gets called. I consider this a big
> advantage.

You're supposed to know what you're doing though and unsharing just because of 
a single call to canonicalize_change_group in fold_rtx seems really overkill.

> Otherwise we would have to look at every validate_change invocation in
> order to check if it might get called after validate_unshare_change. This
> sounds tedious and error-prone to me. (and probably we would end up using
> validate_unshare_change everywhere)

Well, this essentially works fine as of this writing so it's a little hard to 
buy this everything-is-probably-broken argument.  canonicalize_change_group is 
special since it's part of the recog.c API; it needs to support intertwined 
calls with validate_change and validate_unshare_change.  Once this is done, 
it's reasonable to expect that the user of the API doesn't do crazy things.

-- 
Eric Botcazou