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

[Richard Sandiford <rdsandiford at googlemail dot com>]

Paolo Bonzini <paolo.bonzini@gmail.com> 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