[i386 PATCH] Disallow (set reg (op mem const)) addressing modes (take 2)

[Roger Sayle]

Hi Jan,

On Sat, January 20, 2007 4:56 pm, Jan Hubicka wrote:
>> The problem is:
>>
>> (insn 10 7 11 2 (parallel [
>>             (set (reg:SI 61)
>>                 (xor:SI (mem/c/i:SI (reg/f:SI 16 argp) [2 x+0 S4 A32])
>>                     (const_int 4 [0x4])))
>>             (clobber (reg:CC 17 flags))
>>         ]) 233 {*xorsi_1} (nil)
>>     (expr_list:REG_UNUSED (reg:CC 17 flags)
>>         (nil)))
>
> Thanks, this is good catch
> ...
> The conditional looks pretty dubious at the first sight (ie one would
> expect rtx_equal_p and handling of commutative operands like in other
> conditionas in the function).

At this point in ix86_fixup_binary_operands, the operand order has already
been fixed up to place any immediate operand second and any potentially
matching memory operand first, so the rtx_equal_p and handling of
commutative operands is not necessary.  Indeed several of the other uses
rtx_equal_p in that function are superfluous, i.e. match_memory can never
be two.  However,
I completely agree that as written this function is difficult to read.

> I think it is correct (modulo non-canonical forms, like CONST op MEM we
> shuld not throw to backend at first place), but perhaps the both
> functions might be written in a way first computing dest/op1/op2 and
> then for commutative operands it might try to "fix" the ordering (ie
> swapping if op1 is constant/mem not mathcing dest or op2 memory mathing
> dest).  Followed by the tests on validity of noncommucative operand.

Agreed.  In fact, this also allows us to factorize the "operand order
canonicalization code" into it's own function, and re-use it in both
affected functions; ix86_binary_operator_ok and
ix86_fixup_binary_operands.

I've also included one minor optimization.  Previously, given an operator
where both operands were MEMs we'd convert:

    reg = MEM1 op MEM2

into

    tmp = MEM1
    reg = tmp op MEM2

which is reasonable in the general case, but can be improved when MEM1 ==
MEM2 where we can avoid two reads from memory.  The MEM being buried
within a pattern makes it difficult to CSE/GCSE.  Instead, in the case
where the two memories are the same, we now "fix this up" as:

    tmp = MEM
    reg = tmp op tmp


An implementation as you've suggested is attached below.  This has been
tested on i686-pc-linux-gnu with a full "make bootstrap", all default
languages including Ada, and regression tested with a top-level "make -k
check" with no new failures.

Alas no-one has (yet) volunteered any SPEC benchmark figures for this
change,  but we know it enables at least some additional RTL
optimizations, and you and Uros appear to agree this is an
oversight/bug-fix.

Ok for mainline?


2007-01-23  Roger Sayle  <roger@eyesopen.com>

        * config/i386/i386.c (ix86_swap_binary_operands_p): New helper
        function to simplify/factorize operand order canonicalization.
        (ix86_fixup_binary_operands): Reorganize using the above function.
        (ix86_binary_operator_ok): Likewise.


Roger
--
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patchr3.txt
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20070123/a7e6d539/attachment.txt>