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

[Peter Bergner-2]

On Sat, 2009-06-27 at 16:33 +0200, Paolo Bonzini wrote:
> Peter Bergner wrote:
> > What exactly did you have in mind here?
>
> Look into each MEM looking for PLUSes with the operands in the wrong
> order.  Or:
[snip]
> and then in a machine-dependent reorg pass (which rs6000 currently does
> not have):

Ok, I gave this a whirl and although it bootstrapped with no errors, we
hit a few extra ICE's in the testsuite which I looked at.  The problem
is that your code didn't handle load/store with update instructions.
For example:

    (insn 545 897 898 35 (parallel [
            (set (reg/v:SF 45 13 [orig:255 t ] [255])
                (mem:SF (plus:SI (reg:SI 10 10)
                        (reg/f:SI 9 9 [621])) [2 S4 A32]))
            (set (reg:SI 10 10)
                (plus:SI (reg:SI 10 10)
                    (reg/f:SI 9 9 [621])))
        ]) 404 {*movsf_update1} (nil))

I started to add that support (ie, need to call find_plus_and_frob_them
on the address update portion of the insn), when I realized there is
a problem we cannot handle.  The problem is, the dest reg of the address
update (ie, r10) has to be allocated to the same hard reg as the first
operand, so swapping of load/store with update insns after register
allocation is not allowed.  And since r9 is the pointer, we lose.
Had we done this scan with pseudos just before register allocation, we
would have seen this insn which we could swap (because reg 621 dies
in the insn):

    (insn 545 411 546 35 (parallel [
            (set (reg/v:SF 255 [ t ])
                (mem:SF (plus:SI (reg:SI 408 [ D.2317 ])
                        (reg/f:SI 621)) [2 S4 A32]))
            (set (reg:SI 172 [ ivtmp.27 ])
                (plus:SI (reg:SI 408 [ D.2317 ])
                    (reg/f:SI 621)))
        ]) 404 {*movsf_update1} (expr_list:REG_DEAD (reg/f:SI 621)
        (nil)))

Is there a location before register allocation where we can do this pass
so we can catch these types of update insns?  In fact, I think we'd want
to do our swapping before combine which created these update insns, since
creating an update insn could disallow a swap if say reg 621 above didn't
die in insn 545.


Peter

[Paolo Bonzini-3]

On 07/03/2009 05:16 AM, Peter Bergner wrote:

> On Sat, 2009-06-27 at 16:33 +0200, Paolo Bonzini wrote:
>> Peter Bergner wrote:
>>> What exactly did you have in mind here?
>> Look into each MEM looking for PLUSes with the operands in the wrong
>> order.  Or:
> [snip]
>> and then in a machine-dependent reorg pass (which rs6000 currently does
>> not have):
>
> Ok, I gave this a whirl and although it bootstrapped with no errors, we
> hit a few extra ICE's in the testsuite which I looked at.  The problem
> is that your code didn't handle load/store with update instructions.
> For example:
>
>      (insn 545 897 898 35 (parallel [
>              (set (reg/v:SF 45 13 [orig:255 t ] [255])
>                  (mem:SF (plus:SI (reg:SI 10 10)
>                          (reg/f:SI 9 9 [621])) [2 S4 A32]))
>              (set (reg:SI 10 10)
>                  (plus:SI (reg:SI 10 10)
>                      (reg/f:SI 9 9 [621])))
>          ]) 404 {*movsf_update1} (nil))
>
> I started to add that support (ie, need to call find_plus_and_frob_them
> on the address update portion of the insn), when I realized there is
> a problem we cannot handle.

No, we cannot indeed, and in fact you must prevent going within
expressions in the RTX_AUTOINC class for the same reason.  However,
while RTX_AUTOINC must be ruled out explicitly in
find_mems_and_frob_them (the name was temporary of course...), the above
insn can be "handled" just by using validate_change/apply_change_group,
instead of checking the address manually as I was doing.

> Is there a location before register allocation where we can do this pass
> so we can catch these types of update insns?  In fact, I think we'd want
> to do our swapping before combine which created these update insns, since
> creating an update insn could disallow a swap if say reg 621 above didn't
> die in insn 545.

Hmm maybe with a split?

(define_predicate "extend_operator"
  (match_code "zero_extend,sign_extend"))

(define_split
   [(set (match_operand:INT 0 "gpc_reg_operand" "")
         (match_operand:INT 1 "memory_operand" ""))]
   "TARGET_POWER6"
   [(set (match_dup 0) (match_dup 1))]
{
   for_each_rtx (recog_data.operand_loc[0], find_mems_and_frob_them,
                 NULL);
})

(define_split
   [(set (match_operand:INT 0 "memory_operand" "")
         (match_operand:INT 1 "gpc_reg_operand" ""))]
   "TARGET_POWER6"
   [(set (match_dup 0) (match_dup 1))]
{
   for_each_rtx (recog_data.operand_loc[0], find_mems_and_frob_them,
                 NULL);
})

(define_split
   [(set (match_operand:INT 0 "nonimmediate_operand" "")
         (match_operator:INT 1 "extend_operator"
         [(match_operand:INT 2 "memory_operand" "")]))]
   "TARGET_POWER6"
   [(set (match_dup 0) (match_op_dup 1 [(match_dup 2)]))]
{
   for_each_rtx (recog_data.operand_loc[2], find_mems_and_frob_them,
                 NULL);
})

(define_split
   [(set (match_operand:FP 0 "gpc_reg_operand" "")
         (match_operand:FP 1 "memory_operand" ""))]
   "TARGET_POWER6"
   [(set (match_dup 0) (match_dup 1))]
{
   for_each_rtx (recog_data.operand_loc[1], find_mems_and_frob_them,
                 NULL);
})

(define_split
   [(set (match_operand:FP 0 "memory_operand" "")
         (match_operand:FP 1 "gpc_reg_operand" ""))]
   "TARGET_POWER6"
   [(set (match_dup 0) (match_dup 1))]
{
   for_each_rtx (recog_data.operand_loc[0], find_mems_and_frob_them,
                 NULL);
})

(define_split
   [(set (match_operand:FP 0 "nonimmediate_operand" "")
         (float_extend:FP
         (match_operand:FP 1 "memory_operand" "")))]
   "TARGET_POWER6"
   [(set (match_dup 0) (float_extend:FP (match_dup 1)))]
{
   for_each_rtx (recog_data.operand_loc[1], find_mems_and_frob_them,
                 NULL);
})

Paolo

[Richard Sandiford-5]

Paolo Bonzini <paolo.bonzini@...> writes:

> On 07/03/2009 05:16 AM, Peter Bergner wrote:
>> On Sat, 2009-06-27 at 16:33 +0200, Paolo Bonzini wrote:
>>> Peter Bergner wrote:
>>>> What exactly did you have in mind here?
>>> Look into each MEM looking for PLUSes with the operands in the wrong
>>> order.  Or:
>> [snip]
>>> and then in a machine-dependent reorg pass (which rs6000 currently does
>>> not have):
>>
>> Ok, I gave this a whirl and although it bootstrapped with no errors, we
>> hit a few extra ICE's in the testsuite which I looked at.  The problem
>> is that your code didn't handle load/store with update instructions.
>> For example:
>>
>>      (insn 545 897 898 35 (parallel [
>>              (set (reg/v:SF 45 13 [orig:255 t ] [255])
>>                  (mem:SF (plus:SI (reg:SI 10 10)
>>                          (reg/f:SI 9 9 [621])) [2 S4 A32]))
>>              (set (reg:SI 10 10)
>>                  (plus:SI (reg:SI 10 10)
>>                      (reg/f:SI 9 9 [621])))
>>          ]) 404 {*movsf_update1} (nil))
>>
>> I started to add that support (ie, need to call find_plus_and_frob_them
>> on the address update portion of the insn), when I realized there is
>> a problem we cannot handle.
>
> No, we cannot indeed, and in fact you must prevent going within
> expressions in the RTX_AUTOINC class for the same reason.  However,
> while RTX_AUTOINC must be ruled out explicitly in
> find_mems_and_frob_them (the name was temporary of course...), the above
> insn can be "handled" just by using validate_change/apply_change_group,
> instead of checking the address manually as I was doing.
>
>> Is there a location before register allocation where we can do this pass
>> so we can catch these types of update insns?  In fact, I think we'd want
>> to do our swapping before combine which created these update insns, since
>> creating an update insn could disallow a swap if say reg 621 above didn't
>> die in insn 545.
>
> Hmm maybe with a split?

What do you think about the idea of splitting "is this canonical?"
from "is it normally better to swap these two, all other things being
equal?".  I.e. have two versions of swap_commutative_operands_p, such as
must_swap_commutative_operands_p and prefer_to_swap_commutative_operands_p.
Things like canonicalize_change_group would use the "must" version
whereas simplification routines would generally use the "prefer" version.

I realise having two functions could lead to confusion, but the point
is that we ought to think carefully about what each call is trying
to achieve anyway.

Richard

[Peter Bergner-2]

On Sat, 2009-07-04 at 08:21 +0100, Richard Sandiford wrote:
> Paolo Bonzini <paolo.bonzini@...> writes:
> > Hmm maybe with a split?

I'll give that a test... unless you think Richard's idea below is the
way to go.



> What do you think about the idea of splitting "is this canonical?"
> from "is it normally better to swap these two, all other things being
> equal?".  I.e. have two versions of swap_commutative_operands_p, such as
> must_swap_commutative_operands_p and prefer_to_swap_commutative_operands_p.
> Things like canonicalize_change_group would use the "must" version
> whereas simplification routines would generally use the "prefer" version.

I like this, because it's similar to what we have now, but how does this
solve the m68k problem?  It seems that this would only work if we could
guarantee that must_swap_commutative_operands_p is called after the
prefer_to_swap_commutative_operands_p calls, otherwise, we'll end up
swapping them like we are now.  And looking at canonicalize_change_group
(your one suggested call to must_swap_commutative_operands_p), it seems
to be called only from cse.c, is that really late enough to guarantee
we won't call prefer_to_swap_commutative_operands_p anymore?


Peter

[Richard Sandiford-5]

Peter Bergner <bergner@...> writes:

> On Sat, 2009-07-04 at 08:21 +0100, Richard Sandiford wrote:
>> What do you think about the idea of splitting "is this canonical?"
>> from "is it normally better to swap these two, all other things being
>> equal?".  I.e. have two versions of swap_commutative_operands_p, such as
>> must_swap_commutative_operands_p and prefer_to_swap_commutative_operands_p.
>> Things like canonicalize_change_group would use the "must" version
>> whereas simplification routines would generally use the "prefer" version.
>
> I like this, because it's similar to what we have now, but how does this
> solve the m68k problem?  It seems that this would only work if we could
> guarantee that must_swap_commutative_operands_p is called after the
> prefer_to_swap_commutative_operands_p calls, otherwise, we'll end up
> swapping them like we are now.  And looking at canonicalize_change_group
> (your one suggested call to must_swap_commutative_operands_p), it seems
> to be called only from cse.c, is that really late enough to guarantee
> we won't call prefer_to_swap_commutative_operands_p anymore?

We wouldn't need to guarantee that.  In fact, it would be perfectly
OK to use prefer_to_swap_commutative_operands_p after reload.  E.g. many
post-reload transformations take the form "try making this adjustment to
instruction A.  If it works, keep the new instruction, otherwise revert
back to the original A."  prefer_to_swap_commutative_operands_p can be
used in that situation because it doesn't _require_ the swapped form to
match.  (However, if the change fails, it might be better from a QoI
standpoint to try the unswapped version too, provided that
must_swap_commutative_operands_p returns false.)

What you can't do is use prefer_to_swap_commutative_operands_p
on a pattern and require the result to match.

It could be argued that the m68k testcase is tickling a secondary bug.
The problems is that reload1.c:reload_as_needed has the following
code for AUTO_INC_DEC targets:

      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;

Notice how we're doing a final bit of validation -- constrain_operands --
here rather than in the validate routines.  We're assuming that if this
extra validation fails, we can simply call validate_replace_rtx again
to revert the change.  But this isn't always true, because the
validation routines can "canonicalise" the resulting insn.
It would be more robust (and cleaner) to treat this replacement
in the same way as post-reload ones, and do the constrain_operands
in recog.c:insn_invalid_p.

In practice, the code worked with the old canonicalisation rules
because (a) the original instruction ought to be canonical and
(b) replacing (mem (reg)) with (mem ({pre,post}_{inc,dec} (reg)))
shouldn't make a pattern noncanonical.  But the code still seems a
little flaky.

With the new canonicalisation rules, it's (a) that breaks: the effect of
swapping '%' does _not_ always yield a canonical instruction in cases
where it should (and previously did).  We get away with this on
non-AUTO_INC_DEC targets because we often don't check whether an insn is
canonical after reload.  That's not a feature though: we're just letting
through bugs.

Richard

[Paolo Bonzini-3]

On 07/06/2009 09:09 PM, Peter Bergner wrote:
> On Sat, 2009-07-04 at 08:21 +0100, Richard Sandiford wrote:
>> Paolo Bonzini<paolo.bonzini@...>  writes:
>>> Hmm maybe with a split?
>
> I'll give that a test... unless you think Richard's idea below is the
> way to go.

It might be the way to go for the future, but it's not a quick way to
fix PR37053.  I guess we all know (and svn preserves) the history of
PR28690 and this PR, if someone wants to give it a shot they can look at
the history.

Paolo

[Richard Sandiford-5]

Paolo Bonzini <paolo.bonzini@...> writes:

> On 07/06/2009 09:09 PM, Peter Bergner wrote:
>> On Sat, 2009-07-04 at 08:21 +0100, Richard Sandiford wrote:
>>> Paolo Bonzini<paolo.bonzini@...>  writes:
>>>> Hmm maybe with a split?
>>
>> I'll give that a test... unless you think Richard's idea below is the
>> way to go.
>
> It might be the way to go for the future, but it's not a quick way to
> fix PR37053.

FWIW, I think the reload change I suggested -- doing the quoted
constrain_operands inside recog.c:insn_invalid_p rather than
reload1.c itself -- is probably the best fix for the PR on the
release branches.

Richard

[Paolo Bonzini-3]

> FWIW, I think the reload change I suggested -- doing the quoted
> constrain_operands inside recog.c:insn_invalid_p rather than
> reload1.c itself -- is probably the best fix for the PR on the
> release branches.

Agreed; I was speaking about the must_swap vs. prefer_swap distinction.

Paolo

[Peter Bergner-2]

On Fri, 2009-07-03 at 08:00 +0200, Paolo Bonzini wrote:
> On 07/03/2009 05:16 AM, Peter Bergner wrote:
> > Is there a location before register allocation where we can do this pass
> > so we can catch these types of update insns?  In fact, I think we'd want
> > to do our swapping before combine which created these update insns, since
> > creating an update insn could disallow a swap if say reg 621 above didn't
> > die in insn 545.
>
> Hmm maybe with a split?

Ok, the following patch bootstraps and regtests with no regressions.
To get it to this point, I had to modify rs6000_find_plus_and_frob_them
to call memory_address_p() rather than strict_memory_address_p().
I also had to call df_insn_rescan () after swapping operands, otherwise
we die in IRA while calling df_refs_verify().  I also had to move the
define_split's after the include of the dfp.md file, otherwise
we'd get a "could not split insn" error for the following testcase:

  extern _Decimal128 *b, *c;
  _Decimal128
  add (long idx)
  {
    return b[idx] + c[idx];
  }

I think we need the splits after all of the mov* patterns.
In this case movdd_hardfloat32.  Even after all that, this patch
cannot optimize the following simple test case:

  int
  indexedload (int *base, int idx0)
  {
    return base[idx0];
  }

Before combine (and split1), we have this RTL:

  (insn 2 5 3 2 foo.i:3 (set (reg/v/f:SI 124 [ base ])
        (reg:SI 3 3 [ base ])) 340 {*movsi_internal1} (expr_list:REG_DEAD (reg:SI 3 3 [ base ])
        (nil)))

  [snip]

  (insn 9 7 14 2 foo.i:3 (set (reg/f:SI 129)
        (mem/f:SI (plus:SI (reg:SI 127)
                (reg/v/f:SI 124 [ base ])) [0 S4 A32])) 340 {*movsi_internal1} (expr_list:REG_DEAD (reg:SI 127)
        (expr_list:REG_DEAD (reg/v/f:SI 124 [ base ])
            (nil))))

However, after combine, we replace reg/v/f:SI 124 with reg:SI 3
(the incoming arg reg) to get:

  (insn 14 9 17 2 foo.i:5 (set (reg/i:SI 3 3)
        (mem/f:SI (plus:SI (reg:SI 127)
                (reg:SI 3 3 [ base ])) [0 S4 A32])) 340 {*movsi_internal1} (expr_list:REG_DEAD (reg:SI 127)
        (nil)))

Note that the hard reg 3 doesn't...and cannot have the REG_POINTER
attribute, so when we attempt to split, we don't see that we need
to swap operands here.  This works with the curent code, because
when we do our swapping, we're still working with the pseudo 124.
Given this, I don't think the define_split idea is going to work
for PPC.

I guess this leaves us with Richard's idea of modifying reload.
I'll admit I'm not sure what Richard's idea is, so maybe one of
you can explain so I can give a shot at implementing it?


Peter



Index: rtlanal.c
===================================================================
--- rtlanal.c (revision 149345)
+++ rtlanal.c (working copy)
@@ -2935,10 +2935,7 @@ commutative_operand_precedence (rtx op)
 
     case RTX_OBJ:
       /* Complex expressions should be the first, so decrease priority
-         of objects.  Prefer pointer objects over non pointer objects.  */
-      if ((REG_P (op) && REG_POINTER (op))
-  || (MEM_P (op) && MEM_POINTER (op)))
- return -1;
+         of objects.  */
       return -2;
 
     case RTX_COMM_ARITH:
Index: config/rs6000/split.md
===================================================================
--- config/rs6000/split.md (revision 0)
+++ config/rs6000/split.md (revision 0)
@@ -0,0 +1,78 @@
+;; Splits to reorder indexed load/store operands.
+;; Copyright (C) 2009
+;; Free Software Foundation, Inc.
+
+;; This file is part of GCC.
+
+;; GCC is free software; you can redistribute it and/or modify it
+;; under the terms of the GNU General Public License as published
+;; by the Free Software Foundation; either version 3, or (at your
+;; option) any later version.
+
+;; GCC is distributed in the hope that it will be useful, but WITHOUT
+;; ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+;; or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
+;; License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GCC; see the file COPYING3.  If not see
+;; <http://www.gnu.org/licenses/>.
+
+(define_predicate "extend_operator"
+  (match_code "zero_extend,sign_extend"))
+
+(define_split
+   [(set (match_operand:INT 0 "gpc_reg_operand" "")
+         (match_operand:INT 1 "memory_operand" ""))]
+   ""
+   [(set (match_dup 0) (match_dup 1))]
+{
+   rs6000_scan_mems_for_swapping (&operands[1], curr_insn);
+})
+
+(define_split
+   [(set (match_operand:INT 0 "memory_operand" "")
+         (match_operand:INT 1 "gpc_reg_operand" ""))]
+   ""
+   [(set (match_dup 0) (match_dup 1))]
+{
+   rs6000_scan_mems_for_swapping (&operands[0], curr_insn);
+})
+
+(define_split
+   [(set (match_operand:INT 0 "nonimmediate_operand" "")
+         (match_operator:INT 1 "extend_operator"
+ [(match_operand:INT 2 "memory_operand" "")]))]
+   ""
+   [(set (match_dup 0) (match_op_dup 1 [(match_dup 2)]))]
+{
+   rs6000_scan_mems_for_swapping (&operands[2], curr_insn);
+})
+
+(define_split
+   [(set (match_operand:FP 0 "gpc_reg_operand" "")
+         (match_operand:FP 1 "memory_operand" ""))]
+   ""
+   [(set (match_dup 0) (match_dup 1))]
+{
+   rs6000_scan_mems_for_swapping (&operands[1], curr_insn);
+})
+
+(define_split
+   [(set (match_operand:FP 0 "memory_operand" "")
+         (match_operand:FP 1 "gpc_reg_operand" ""))]
+   ""
+   [(set (match_dup 0) (match_dup 1))]
+{
+   rs6000_scan_mems_for_swapping (&operands[0], curr_insn);
+})
+
+(define_split
+   [(set (match_operand:FP 0 "nonimmediate_operand" "")
+         (float_extend:FP
+ (match_operand:FP 1 "memory_operand" "")))]
+   ""
+   [(set (match_dup 0) (float_extend:FP (match_dup 1)))]
+{
+   rs6000_scan_mems_for_swapping (&operands[1], curr_insn);
+})
Index: config/rs6000/rs6000-protos.h
===================================================================
--- config/rs6000/rs6000-protos.h (revision 149345)
+++ config/rs6000/rs6000-protos.h (working copy)
@@ -179,6 +179,8 @@ extern int rs6000_memory_move_cost (enum
 extern bool rs6000_tls_referenced_p (rtx);
 extern void rs6000_conditional_register_usage (void);
 
+extern void rs6000_scan_mems_for_swapping (rtx *, rtx);
+
 /* Declare functions in rs6000-c.c */
 
 extern void rs6000_pragma_longcall (struct cpp_reader *);
Index: config/rs6000/t-rs6000
===================================================================
--- config/rs6000/t-rs6000 (revision 149345)
+++ config/rs6000/t-rs6000 (working copy)
@@ -62,4 +62,5 @@ MD_INCLUDES = $(srcdir)/config/rs6000/ri
  $(srcdir)/config/rs6000/altivec.md \
  $(srcdir)/config/rs6000/spe.md \
  $(srcdir)/config/rs6000/dfp.md \
- $(srcdir)/config/rs6000/paired.md
+ $(srcdir)/config/rs6000/paired.md \
+ $(srcdir)/config/rs6000/split.md
Index: config/rs6000/rs6000.c
===================================================================
--- config/rs6000/rs6000.c (revision 149345)
+++ config/rs6000/rs6000.c (working copy)
@@ -24023,4 +24023,71 @@ rs6000_final_prescan_insn (rtx insn, rtx
     }
 }
 
+static bool rs6000_rescan;
+
+static bool
+rs6000_is_pointer_object (rtx x)
+{
+  if (REG_P (x))
+    return REG_POINTER (x);
+  if (MEM_P (x))
+    return MEM_POINTER (x);
+  return false;
+}
+
+/* We are inside a MEM, find PLUSes were the index comes first and
+   invert the base and index.  DATA points to the mem within which
+   *PX lies, which is used to validate the address.  */
+
+static int
+rs6000_find_plus_and_frob_them (rtx *px, void *data)
+{
+  if (GET_CODE (*px) == PLUS
+      && OBJECT_P (XEXP (*px, 1))
+      && OBJECT_P (XEXP (*px, 0))
+      && rs6000_is_pointer_object (XEXP (*px, 1))
+      && !rs6000_is_pointer_object (XEXP (*px, 0)))
+    {
+      rtx mem = (rtx)data;
+      rtx t = XEXP (*px, 0);
+      XEXP (*px, 0) = XEXP (*px, 1);
+      XEXP (*px, 1) = t;
+      gcc_assert (!swap_commutative_operands_p (XEXP (*px, 0), XEXP (*px, 1)));
+      if (!memory_address_p (GET_MODE (mem), XEXP (mem, 0)))
+ {
+  /* Too bad, probably XEXP (*px, 0) is r0 and cannot be made into
+     an index.  Undo.  */
+  XEXP (*px, 1) = XEXP (*px, 0);
+  XEXP (*px, 0) = t;
+ }
+      else
+ rs6000_rescan = true;
+    }
+  return 0;
+}
+
+/* Look for memory operands and apply rs6000_find_plus_and_frob_them
+   to their addresses.  */
+static int
+rs6000_find_mems_and_frob_them (rtx *px, void *data ATTRIBUTE_UNUSED)
+{
+  if (*px != NULL && GET_CODE (*px) == MEM)
+    {
+      for_each_rtx (&XEXP (*px, 0), rs6000_find_plus_and_frob_them, *px);
+      return -1;
+    }
+  return 0;
+}
+
+void
+rs6000_scan_mems_for_swapping (rtx *op, rtx insn)
+{
+  rs6000_rescan = false;
+
+  for_each_rtx (op, rs6000_find_mems_and_frob_them, NULL);
+
+  if (rs6000_rescan)
+    df_insn_rescan (insn);
+}
+
 #include "gt-rs6000.h"
Index: config/rs6000/rs6000.md
===================================================================
--- config/rs6000/rs6000.md (revision 149345)
+++ config/rs6000/rs6000.md (working copy)
@@ -15326,3 +15326,4 @@ (define_insn "prefetch"
 (include "spe.md")
 (include "dfp.md")
 (include "paired.md")
+(include "split.md")

[Paolo Bonzini-2]

> Notice how we're doing a final bit of validation -- constrain_operands --
> here rather than in the validate routines.  We're assuming that if this
> extra validation fails, we can simply call validate_replace_rtx again
> to revert the change.  But this isn't always true, because the
> validation routines can "canonicalise" the resulting insn.
> It would be more robust (and cleaner) to treat this replacement
> in the same way as post-reload ones, and do the constrain_operands
> in recog.c:insn_invalid_p.

Note that it is already being done if reload_completed, just not if
reload_in_progress.  No matter how pleasant, it would be a surprise if
the attached patch worked without this change it includes:

-      if (! constrain_operands (1))
+      if (! constrain_operands (reload_completed))

(Note: I haven't tested it with the change, either).  On the other hand,
with the change there is a subtle difference in reload_as_needed now
(I'm not sure it matters).

Maxim, can you try it on m68k?

Paolo

Index: /home/pbonzini/devel/gcc/gcc/reload1.c
===================================================================
--- /home/pbonzini/devel/gcc/gcc/reload1.c (revision 149170)
+++ /home/pbonzini/devel/gcc/gcc/reload1.c (working copy)
@@ -4310,25 +4310,8 @@
        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;
  }
Index: /home/pbonzini/devel/gcc/gcc/recog.c
===================================================================
--- /home/pbonzini/devel/gcc/gcc/recog.c (revision 149170)
+++ /home/pbonzini/devel/gcc/gcc/recog.c (working copy)
@@ -328,11 +328,11 @@
     }
 
   /* After reload, verify that all constraints are satisfied.  */
-  if (reload_completed)
+  if (reload_in_progress || reload_completed)
     {
       extract_insn (insn);
 
-      if (! constrain_operands (1))
+      if (! constrain_operands (reload_completed))
  return 1;
     }
 

[Richard Sandiford-5]

2009/7/9 Paolo Bonzini <bonzini@...>:

>
>> Notice how we're doing a final bit of validation -- constrain_operands --
>> here rather than in the validate routines.  We're assuming that if this
>> extra validation fails, we can simply call validate_replace_rtx again
>> to revert the change.  But this isn't always true, because the
>> validation routines can "canonicalise" the resulting insn.
>> It would be more robust (and cleaner) to treat this replacement
>> in the same way as post-reload ones, and do the constrain_operands
>> in recog.c:insn_invalid_p.
>
> Note that it is already being done if reload_completed, just not if
> reload_in_progress.

Right, that's what I meant by "in the same way as the post-reload
ones".  Sorry for the confusion!

Richard

[Peter Bergner-2]

On Fri, 2009-07-10 at 00:58 +0200, Paolo Bonzini wrote:

> Note that it is already being done if reload_completed, just not if
> reload_in_progress.  No matter how pleasant, it would be a surprise if
> the attached patch worked without this change it includes:
>
> -      if (! constrain_operands (1))
> +      if (! constrain_operands (reload_completed))
>
> (Note: I haven't tested it with the change, either).  On the other hand,
> with the change there is a subtle difference in reload_as_needed now
> (I'm not sure it matters).
>
> Maxim, can you try it on m68k?

FYI, this bootstrapped and regtested with no errors on powerpc64-linux.

Peter

[Paolo Bonzini-2]

On 07/10/2009 03:14 PM, Peter Bergner wrote:

> On Fri, 2009-07-10 at 00:58 +0200, Paolo Bonzini wrote:
>> Note that it is already being done if reload_completed, just not if
>> reload_in_progress.  No matter how pleasant, it would be a surprise if
>> the attached patch worked without this change it includes:
>>
>> -      if (! constrain_operands (1))
>> +      if (! constrain_operands (reload_completed))
>>
>> (Note: I haven't tested it with the change, either).  On the other hand,
>> with the change there is a subtle difference in reload_as_needed now
>> (I'm not sure it matters).
>>
>> Maxim, can you try it on m68k?
>
> FYI, this bootstrapped and regtested with no errors on powerpc64-linux.

[patch at http://permalink.gmane.org/gmane.comp.gcc.patches/189115]

It also fixes the testcase.  Ok for mainline and branches?

Paolo

[Richard Sandiford-5]

2009/7/14 Paolo Bonzini <bonzini@...>:

> On 07/10/2009 03:14 PM, Peter Bergner wrote:
>>
>> On Fri, 2009-07-10 at 00:58 +0200, Paolo Bonzini wrote:
>>>
>>> Note that it is already being done if reload_completed, just not if
>>> reload_in_progress.  No matter how pleasant, it would be a surprise if
>>> the attached patch worked without this change it includes:
>>>
>>> -      if (! constrain_operands (1))
>>> +      if (! constrain_operands (reload_completed))
>>>
>>> (Note: I haven't tested it with the change, either).  On the other hand,
>>> with the change there is a subtle difference in reload_as_needed now
>>> (I'm not sure it matters).
>>>
>>> Maxim, can you try it on m68k?
>>
>> FYI, this bootstrapped and regtested with no errors on powerpc64-linux.
>
> [patch at http://permalink.gmane.org/gmane.comp.gcc.patches/189115]
>
> It also fixes the testcase.  Ok for mainline and branches?

Sorry to butt in again, but like you say, I'm worried about changing
the contrain_operands argument from 1 to 0.  The reload1.c code
that we've been talking about runs after the main reload processing,
so we shouldn't accept anything that doesn't strictly match its constraints.
We might also be pessimising things by changing the behaviour of
eliminate_regs_in_insn, although I haven't really thought about that much.

I know it's ugly, but how about either: (a) temporarily setting
reload_completed around the autoinc validate_change or
(b) adding a new interface to select what we want, with the
default being the same as now?  E.g. (b) could take a callback
that does the validation, which would actually make the routines
a bit more flexible.

As penance for yakking so much, I'll do a patch for (b) if that
sounds reasonable.

Richard

[Maxim Kuvyrkov-2]

Paolo Bonzini wrote:

> On 07/10/2009 03:14 PM, Peter Bergner wrote:
>> On Fri, 2009-07-10 at 00:58 +0200, Paolo Bonzini wrote:
>>> Note that it is already being done if reload_completed, just not if
>>> reload_in_progress.  No matter how pleasant, it would be a surprise if
>>> the attached patch worked without this change it includes:
>>>
>>> -      if (! constrain_operands (1))
>>> +      if (! constrain_operands (reload_completed))
>>>
>>> (Note: I haven't tested it with the change, either).  On the other hand,
>>> with the change there is a subtle difference in reload_as_needed now
>>> (I'm not sure it matters).
>>>
>>> Maxim, can you try it on m68k?
>>
>> FYI, this bootstrapped and regtested with no errors on powerpc64-linux.
>
> [patch at http://permalink.gmane.org/gmane.comp.gcc.patches/189115]
>
> It also fixes the testcase.  Ok for mainline and branches?

Sorry it took me so long to reply.

The above patch doesn't fix the ICE on m68k.  Compiling the preprocessed
testcase in PR37053 yields:

./cc1 -fpic -O1 /tmp/postreload.c -o /tmp/postreload.s -m68020

...

postreload.c: In function 'do_termsform':
postreload.c:886:1: error: insn does not satisfy its constraints:
(insn 480 3109 3111 34 postreload.c:446 (set (reg:SI 0 %d0)
         (plus:SI (mem/f:SI (post_inc:SI (reg:SI 8 %a0)) [0 S4 A16])
             (reg:SI 0 %d0))) 140 {*addsi3_internal} (expr_list:REG_INC
(reg:SI 8 %a0)
         (nil)))
postreload.c:886:1: internal compiler error: in
reload_cse_simplify_operands, at postreload.c:396

Note that -m68020 option is necessary to trigger the bug.

--
Maxim

[Richard Sandiford-5]

Richard Sandiford <rdsandiford@...> writes:

> 2009/7/14 Paolo Bonzini <bonzini@...>:
>> On 07/10/2009 03:14 PM, Peter Bergner wrote:
>>>
>>> On Fri, 2009-07-10 at 00:58 +0200, Paolo Bonzini wrote:
>>>>
>>>> Note that it is already being done if reload_completed, just not if
>>>> reload_in_progress.  No matter how pleasant, it would be a surprise if
>>>> the attached patch worked without this change it includes:
>>>>
>>>> -      if (! constrain_operands (1))
>>>> +      if (! constrain_operands (reload_completed))
>>>>
>>>> (Note: I haven't tested it with the change, either).  On the other hand,
>>>> with the change there is a subtle difference in reload_as_needed now
>>>> (I'm not sure it matters).
>>>>
>>>> Maxim, can you try it on m68k?
>>>
>>> FYI, this bootstrapped and regtested with no errors on powerpc64-linux.
>>
>> [patch at http://permalink.gmane.org/gmane.comp.gcc.patches/189115]
>>
>> It also fixes the testcase.  Ok for mainline and branches?
>
> Sorry to butt in again, but like you say, I'm worried about changing
> the contrain_operands argument from 1 to 0.  The reload1.c code
> that we've been talking about runs after the main reload processing,
> so we shouldn't accept anything that doesn't strictly match its constraints.
> We might also be pessimising things by changing the behaviour of
> eliminate_regs_in_insn, although I haven't really thought about that much.
>
> I know it's ugly, but how about either: (a) temporarily setting
> reload_completed around the autoinc validate_change or
> (b) adding a new interface to select what we want, with the
> default being the same as now?  E.g. (b) could take a callback
> that does the validation, which would actually make the routines
> a bit more flexible.

Still a bit rough, but how about this?  In the end I just added an
enum rather than a hook.  I suppose the enum could also be retrofitted
elsewhere, such as the parameter to constrain_operands, but I wanted
to keep it simple.

Not tested beyond the PR testcase.

Richard


gcc/
        * recog.h (recog_strictness): New enum.
        * recog.c (validation_strictness): New variable.
        (insn_invalid_p): Check it.
        (push_validation_strictness): New function.
        (pop_validation_strictness): Likewise.
        * reload1.c (reload_as_needed): Use RS_STRICT_CONSTRAINTS.

Index: gcc/recog.h
===================================================================
--- gcc/recog.h 2009-07-14 21:02:54.000000000 +0100
+++ gcc/recog.h 2009-07-14 21:11:52.000000000 +0100
@@ -68,9 +68,30 @@ struct operand_alternative
   unsigned int anything_ok:1;
 };
 
+/* Enumerates how strict we can be when matching instructions.  */
+enum recog_strictness {
+  /* The default behavior: RS_STRICT_CONSTRAINTS after reload and
+     RS_NO_CONSTRAINTS otherwise.  */
+  RS_DEFAULT,
+
+  /* Only check whether a pattern is an asm or matches a define_insn;
+     don't care about whether the constraints are satisfied.  */
+  RS_NO_CONSTRAINTS,
+
+  /* As for RS_NO_CONSTRAINTS, but also require the constraints to
+     be satisfied in a non-strict sense.  (See constrain_operands for
+     details about strictness.)  */
+  RS_LOOSE_CONSTRAINTS,
+
+  /* As for RS_NO_CONSTRAINTS, but also require the constraints
+     to be satisfied in a strict sense.  */
+  RS_STRICT_CONSTRAINTS
+};
 
 extern void init_recog (void);
 extern void init_recog_no_volatile (void);
+extern enum recog_strictness push_validation_strictness (enum recog_strictness);
+extern void pop_validation_strictness (enum recog_strictness);
 extern int check_asm_operands (rtx);
 extern int asm_operand_ok (rtx, const char *, const char **);
 extern bool validate_change (rtx, rtx *, rtx, bool);
Index: gcc/recog.c
===================================================================
--- gcc/recog.c 2009-07-14 21:02:54.000000000 +0100
+++ gcc/recog.c 2009-07-14 21:05:11.000000000 +0100
@@ -179,6 +179,9 @@ typedef struct change_t
 
 static int num_changes = 0;
 
+/* How strict the matching should be.  */
+static enum recog_strictness validation_strictness;
+
 /* Validate a proposed change to OBJECT.  LOC is the location in the rtl
    at which NEW_RTX will be placed.  If OBJECT is zero, no validation is done,
    the change is simply made.
@@ -303,7 +306,7 @@ insn_invalid_p (rtx insn)
       && ! reload_completed && ! reload_in_progress)
      ? &num_clobbers : 0);
   int is_asm = icode < 0 && asm_noperands (PATTERN (insn)) >= 0;
-
+  enum recog_strictness strictness;
 
   /* If this is an asm and the operand aren't legal, then fail.  Likewise if
      this is not an asm and the insn wasn't recognized.  */
@@ -327,12 +330,16 @@ insn_invalid_p (rtx insn)
       PATTERN (insn) = pat = newpat;
     }
 
-  /* After reload, verify that all constraints are satisfied.  */
-  if (reload_completed)
+  /* Optionally verify that all constraints are satisfied.  */
+  strictness = validation_strictness;
+  if (strictness == RS_DEFAULT)
+    strictness = reload_completed ? RS_STRICT_CONSTRAINTS : RS_NO_CONSTRAINTS;
+
+  if (strictness != RS_NO_CONSTRAINTS)
     {
       extract_insn (insn);
 
-      if (! constrain_operands (1))
+      if (! constrain_operands (strictness == RS_STRICT_CONSTRAINTS))
  return 1;
     }
 
@@ -347,6 +354,32 @@ num_changes_pending (void)
   return num_changes;
 }
 
+/* Switch to a new validation strictness for a period of time.
+   This controls the strictness of insn_invalid_p, the validate_*
+   routines, and apply_change_group.
+
+   Pass the returned value to pop_validation_strictness to return
+   to the current status.  */
+
+enum recog_strictness
+push_validation_strictness (enum recog_strictness new_strictness)
+{
+  enum recog_strictness old_strictness;
+
+  old_strictness = validation_strictness;
+  validation_strictness = new_strictness;
+  return old_strictness;
+}
+
+/* Return to the status before the last push_validation_strictness.
+   OLD_STRICTNESS is the value returned by that function.  */
+
+void
+pop_validation_strictness (enum recog_strictness old_strictness)
+{
+  validation_strictness = old_strictness;
+}
+
 /* Tentatively apply the changes numbered NUM and up.
    Return 1 if all changes are valid, zero otherwise.  */
 
Index: gcc/reload1.c
===================================================================
--- gcc/reload1.c 2009-07-14 21:02:54.000000000 +0100
+++ gcc/reload1.c 2009-07-14 21:05:11.000000000 +0100
@@ -4304,31 +4304,16 @@ reload_as_needed (int live_known)
     continue;
   if (n == 1)
     {
+      enum recog_strictness os;
+
+      os = (push_validation_strictness
+    (RS_STRICT_CONSTRAINTS));
       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;
- }
-
+      pop_validation_strictness (os);
     }
   break;
  }

[Paolo Bonzini-2]

What about this?  A bit simpler. :-)

(And this time I tested it with the right options).

Paolo

2009-07-14  Paolo Bonzini  <bonzini@...>

        * reload1.c (reload_as_needed): Set reload_completed to
        ensure validate_replace_rtx processes constraints strictly.

Index: reload1.c
===================================================================
--- reload1.c (revision 149508)
+++ reload1.c (working copy)
@@ -4304,31 +4304,19 @@ reload_as_needed (int live_known)
     continue;
   if (n == 1)
     {
+      /* We want validate_replace_rtx to only succeed
+ if this insn satisfies its constraints
+ strictly.  */
+      reload_completed = 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);
+      reload_completed = 0;
       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;
  }

[Richard Sandiford-5]

> What about this?  A bit simpler. :-)

Yeah, this was option (a) from yesterday's message, but I preferred
(b) because it seemed less hackish.  I don't like the idea of
saying reload is completed when in fact it's still in progress,
or introducing a case where reload_completed and
reload_in_progress are _both_ true.

But I think we're at the stage where we need a maintainer
to comment ;)

Richard

[Peter Bergner-2]

On Tue, 2009-07-14 at 21:13 +0100, Richard Sandiford wrote:
> Still a bit rough, but how about this?  In the end I just added an
> enum rather than a hook.  I suppose the enum could also be retrofitted
> elsewhere, such as the parameter to constrain_operands, but I wanted
> to keep it simple.
>
> Not tested beyond the PR testcase.

This bootstrapped and regtested with no regressions on powerpc64-linux.

Peter

[Peter Bergner-2]

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