Re: [PATCH]: PR 11271 Handle reloading of R+R+CONST for any reg

[Ulrich Weigand <weigand at i1 dot informatik dot uni-erlangen dot de>]

Eric Botcazou wrote:

> > No, it's an essential part of recovering from register elimination, and
> > was causing a bootstrap failure when building the Thumb libraries for
> > gcc/ARM
> 
> Yes, it is necessary on SPARC too because of preventive subreg big-endian 
> corrections, as indicated in the comment.
> 
> However your patch looks suspicious to me too, because the code really builds 
> a (base + index) pattern, so I think it makes sense to test that it is 
> possible to do so beforehand.

I think there are really two separate issues.  The one is a question of
optimization: is splitting out the REG + CONST into a separate reload
beneficial on the particular platform?  On s390, this is likely not to
be true in the case of (non-base REG + REG + CONST), so I don't want
that case to hit in any case.   One possible fix for this might be:

 else if (GET_CODE (ad) == PLUS && GET_CODE (XEXP (ad, 1)) == CONST_INT
           && GET_CODE (XEXP (ad, 0)) == PLUS
           && GET_CODE (XEXP (XEXP (ad, 0), 0)) == REG
           && REGNO (XEXP (XEXP (ad, 0), 0)) < FIRST_PSEUDO_REGISTER
+          && ! maybe_memory_address_p (mode, ad, &XEXP (XEXP (ad, 0), 0)))
           && ! maybe_memory_address_p (mode, ad, &XEXP (XEXP (ad, 0), 1)))
    {

i.e. if the whole address gets valid by reloading either of the summands,
then just doing so will be better than performing the split.  On arm
the split would be performed anyway because the three-way sum isn't
valid even after register reloading.


The other issue is a correctness issue: if find_reloads_address decides
to perform the split, everything should continue to work.  I've looked
a bit more into why it currently breaks on s390.  On the first call to
find_reloads, I get this insn:

(insn 102 101 104 15 ../../gcc-head/gcc/gcse.c:740 (set (mem/s:DI (plus:SI (plus:SI (reg:SI %r0 [81])
                    (reg/f:SI %r4 [77]))
                (const_int 16 [0x10])) [21 <variable>.elms S8 A64])
        (reg:DI 95)) 54 {*movdi_31} (insn_list 101 (insn_list 84 (insn_list 81 (insn_list:REG_DEP_ANTI 75 (insn_list:REG_DEP_ANTI 67 (insn_list:REG_DEP_ANTI 97 (nil)))))))
    (expr_list:REG_DEAD (reg:DI 95)
        (expr_list:REG_DEAD (reg:SI %r0 [81])
            (expr_list:REG_DEAD (reg/f:SI %r4 [77])
                (nil)))))

which after reload was transformed to:

(insn 102 101 104 15 ../../gcc-head/gcc/gcse.c:740 (set (mem/s:DI (plus:SI (plus:SI (reg:SI %r0 [81])
                    (const_int 16 [0x10]))
                (reg/f:SI %r4 [77])) [21 <variable>.elms S8 A64])
        (reg:DI 95)) 54 {*movdi_31} (insn_list 101 (insn_list 84 (insn_list 81 (insn_list:REG_DEP_ANTI 75 (insn_list:REG_DEP_ANTI 67 (insn_list:REG_DEP_ANTI 97 (nil)))))))
    (expr_list:REG_DEAD (reg:DI 95)
        (expr_list:REG_DEAD (reg:SI %r0 [81])
            (expr_list:REG_DEAD (reg/f:SI %r4 [77])
                (nil)))))

with those reloads pushed:

Reload 0: ADDR_REGS, RELOAD_FOR_OUTPUT_ADDRESS (opnum = 0), can't combine, secondary_reload_p
Reload 1: reload_in (SI) = (plus:SI (reg:SI %r0 [81])
                                                    (const_int 16 [0x10]))
        ADDR_REGS, RELOAD_FOR_OUTPUT_ADDRESS (opnum = 0)
        reload_in_reg: (plus:SI (reg:SI %r0 [81])
                                                    (const_int 16 [0x10]))
        secondary_in_reload = 0
        secondary_in_icode = reload_insi
Reload 2: ADDR_REGS, RELOAD_FOR_OUTPUT_ADDRESS (opnum = 0), can't combine, secondary_reload_p
Reload 3: reload_out (DI) = (mem/s:DI (plus:SI (plus:SI (reg:SI %r0 [81])
                                                            (const_int 16 [0x10]))
                                                        (reg/f:SI %r4 [77])) [21 <variable>.elms S8 A64])
        GENERAL_REGS, RELOAD_FOR_OUTPUT (opnum = 0)
        reload_out_reg: (mem/s:DI (plus:SI (plus:SI (reg:SI %r0 [81])
                                                            (const_int 16 [0x10]))
                                                        (reg/f:SI %r4 [77])) [21 <variable>.elms S8 A64])
        secondary_out_reload = 2

The secondary output reload would not be strictly necessary, but is triggered
because (plus:SI (plus:SI (reg 0) (const_int 16)) (reg 4)) is not offsettable
(because it is in fact not even a valid address ...).  However, it should not
do any actual harm ...

Now, on the *second* pass through find_reloads, the change made to the insn
itself was retained; now the check in find_reloads_address lines 4913 etc.
now longer matches!

This means I still get this insn:

(insn 102 101 104 15 ../../gcc-head/gcc/gcse.c:740 (set (mem/s:DI (plus:SI (plus:SI (reg:SI %r0 [81])
                    (const_int 16 [0x10]))
                (reg/f:SI %r10 [77])) [21 <variable>.elms S8 A64])
        (reg:DI 95)) 54 {*movdi_31} (insn_list 101 (insn_list 84 (insn_list 81 (insn_list:REG_DEP_ANTI 75 (insn_list:REG_DEP_ANTI 67 (insn_list:REG_DEP_ANTI 97 (nil)))))))
    (expr_list:REG_DEAD (reg:DI 95)
        (expr_list:REG_DEAD (reg:SI %r0 [81])
            (expr_list:REG_DEAD (reg/f:SI %r10 [77])
                (nil)))))

but only those reloads:

Reload 0: reload_in (SI) = (reg:SI %r0 [81])
        ADDR_REGS, RELOAD_FOR_OUTPUT_ADDRESS (opnum = 0)
        reload_in_reg: (reg:SI %r0 [81])
Reload 1: ADDR_REGS, RELOAD_FOR_OUTPUT_ADDRESS (opnum = 0), can't combine, secondary_reload_p
Reload 2: reload_out (DI) = (mem/s:DI (plus:SI (plus:SI (reg:SI %r0 [81])
                                                            (const_int 16 [0x10]))
                                                        (reg/f:SI %r10 [77])) [21 <variable>.elms S8 A64])
        GENERAL_REGS, RELOAD_FOR_OUTPUT (opnum = 0)
        reload_out_reg: (mem/s:DI (plus:SI (plus:SI (reg:SI %r0 [81])
                                                            (const_int 16 [0x10]))
                                                        (reg/f:SI %r10 [77])) [21 <variable>.elms S8 A64])
        secondary_out_reload = 1

The reload that was supposed to fix up the invalid address has disappeared.
The processing of the secondary output reload now inserts an insn 
containing that invalid address, which causes the ICE later on.


Now, what looks broken to me in this scenario is that I thought all changes
done by find_reloads should be reverted in the calculate_needs_all_insns
loop, so that each pass through the loop finds the original insn again.

However, it turns out this is done *only* if register eliminations were
performed -- apparently the thought is find_reloads only needs to do 
strange things to clean up after eliminations, so if there were none,
then find_reloads shouldn't modifiy the insn either.

In the case that hits on s390, there were no eliminations involved, however.
This is simply the result of register allocation assigning a non-base reg
to a pseudo, which -while certainly not optimal- shouldn't break either.


If this explanation is correct, then maybe the patch shown above would
actually fix the correctness issue as well, because now the 'if' would
apply only if more than a simple register reload is required to arrive
at a valid address -- and this means the address must have been broken
by register elimination, right?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de