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

[Richard Sandiford <rdsandiford at googlemail dot com>]

Peter Bergner <bergner@vnet.ibm.com> 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