[PATCH] Fix PR37053: Move tweaks of commutative precedence to target hook

View: New views
7 Messages — Rating Filter:   Alert me  
< Prev | 1 - 2 - 3 | Next >

Re: [PATCH] Fix PR37053: Move tweaks of commutative precedence to target hook

by Paolo Bonzini-3 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On 07/16/2009 06:44 AM, Peter Bergner wrote:

> On Tue, 2009-07-14 at 23:09 +0200, Paolo Bonzini wrote:
>> What about this?  A bit simpler. :-)
>>
>> (And this time I tested it with the right options).
>
> This bootstrapped and regtested with no regressions on powerpc64-linux,
> as did Richard's patch.
>
> I have to agree with Richard, that it seems a little hackish setting
> reload_completed this way.

To some extent, however, I still think my patch is preferrable for
simplicity.  To make it less hackish, let's follow it by a grand
s/reload_completed/rtl_strict_p/g...

Paolo

Re: [PATCH] Fix PR37053: Move tweaks of commutative precedence to target hook

by Ulrich Weigand :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Peter Bergner wrote:

> On Tue, 2009-07-14 at 23:09 +0200, Paolo Bonzini wrote:
> > What about this?  A bit simpler. :-)
> >
> > (And this time I tested it with the right options).
>
> This bootstrapped and regtested with no regressions on powerpc64-linux,
> as did Richard's patch.
>
> I have to agree with Richard, that it seems a little hackish setting
> reload_completed this way.

Peter asked me to have a look at this problem as well.  I agree that
setting reload_completed here seems a little dangerous.  In particular,
because reload is in fact *not* completed: in particulr, replacement of
pseudo registers by their permanent stack slots has not yet been
performed.  Code that assumes "reload_completed" means that no pseudos
occur in instructions may well get confused by this ...

I'm wondering if we cannot make a simple local fix to the problem
that "validate_replace_rtx" cannot be completely undone by doing
another "validate_replace_rtx".  We should be able to keep the
logic in reload_as_needed as-is, but simply use the existing
change-group management routines to cancel or confirm the change
after we did the extra test ...

The patch below implements this option.  Any comments?  Can you
verify this fixes the original problem?

Bye,
Ulrich

ChangeLog:

        * reload1.c (reload_as_needed): Use cancel_changes to completely
        undo a failed replacement attempt.


Index: gcc/reload1.c
===================================================================
*** gcc/reload1.c (revision 150048)
--- gcc/reload1.c (working copy)
*************** reload_as_needed (int live_known)
*** 4304,4334 ****
     continue;
   if (n == 1)
     {
!      n = validate_replace_rtx (reload_reg,
! gen_rtx_fmt_e (code,
!       mode,
!       reload_reg),
! p);
 
       /* We must also verify that the constraints
  are met after the replacement.  */
       extract_insn (p);
       if (n)
  n = constrain_operands (1);
-      else
- break;
 
       /* If the constraints were not met, then
! undo the replacement.  */
       if (!n)
! {
!  validate_replace_rtx (gen_rtx_fmt_e (code,
!       mode,
!       reload_reg),
! reload_reg, p);
!  break;
! }
!
     }
   break;
  }
--- 4304,4328 ----
     continue;
   if (n == 1)
     {
!      rtx replace_reg
! = gen_rtx_fmt_e (code, mode, reload_reg);
!
!      validate_replace_rtx_group (reload_reg,
!  replace_reg, p);
!      n = verify_changes (0);
 
       /* We must also verify that the constraints
  are met after the replacement.  */
       extract_insn (p);
       if (n)
  n = constrain_operands (1);
 
       /* If the constraints were not met, then
! undo the replacement, else confirm it.  */
       if (!n)
! cancel_changes (0);
!      else
! confirm_change_group ();
     }
   break;
  }


--
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@...

Re: [PATCH] Fix PR37053: Move tweaks of commutative precedence to target hook

by Paolo Bonzini-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message


> I'm wondering if we cannot make a simple local fix to the problem
> that "validate_replace_rtx" cannot be completely undone by doing
> another "validate_replace_rtx".  We should be able to keep the
> logic in reload_as_needed as-is, but simply use the existing
> change-group management routines to cancel or confirm the change
> after we did the extra test ...
>
> The patch below implements this option.  Any comments?  Can you
> verify this fixes the original problem?

If it works (which I'd be pretty confident of), I can only feel stupid. :-)

Paolo

Re: [PATCH] Fix PR37053: Move tweaks of commutative precedence to target hook

by Maxim Kuvyrkov-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Ulrich Weigand wrote:

...

> The patch below implements this option.  Any comments?  Can you
> verify this fixes the original problem?

All the three patches posted by Paolo, Richard and Ulrich fix the
testcase on m68k.  Neither patch was tested beyond that yet.

I've started a full regtest of the Ulrich's patch on ColdFire and will
follow up later.


Thanks,

--
Maxim

Re: [PATCH] Fix PR37053: Move tweaks of commutative precedence to target hook

by Maxim Kuvyrkov-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Maxim Kuvyrkov wrote:

> Ulrich Weigand wrote:
>
> ...
>
>> The patch below implements this option.  Any comments?  Can you
>> verify this fixes the original problem?
>
> All the three patches posted by Paolo, Richard and Ulrich fix the
> testcase on m68k.  Neither patch was tested beyond that yet.
>
> I've started a full regtest of the Ulrich's patch on ColdFire and will
> follow up later.

Ulrich, thanks for the patch; it doesn't cause any regressions on gcc,
g++ and libstdc++ testsuites on coldfire-linux-gnu.

--
Maxim

Re: [PATCH] Fix PR37053: Move tweaks of commutative precedence to target hook

by Ulrich Weigand :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Maxim Kuvyrkov wrote:

> Maxim Kuvyrkov wrote:
> > Ulrich Weigand wrote:
> >
> > ...
> >
> >> The patch below implements this option.  Any comments?  Can you
> >> verify this fixes the original problem?
> >
> > All the three patches posted by Paolo, Richard and Ulrich fix the
> > testcase on m68k.  Neither patch was tested beyond that yet.
> >
> > I've started a full regtest of the Ulrich's patch on ColdFire and will
> > follow up later.
>
> Ulrich, thanks for the patch; it doesn't cause any regressions on gcc,
> g++ and libstdc++ testsuites on coldfire-linux-gnu.

Thanks for testing!  I've now checked the patch in to mainline.

Bye,
Ulrich

--
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@...

Re: [PATCH] Fix PR37053: Move tweaks of commutative precedence to target hook

by Hans-Peter Nilsson-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Wed, 5 Aug 2009, Ulrich Weigand wrote:
> * reload1.c (reload_as_needed): Use cancel_changes to completely
> undo a failed replacement attempt.

This caused PR41064.

brgds, H-P
< Prev | 1 - 2 - 3 | Next >