Re: Re: Unreviewed patch^3

[Andreas Krebbel <krebbel1 at de dot ibm dot com>]

Hi Geoffrey,

thanks for looking into my patch.

I want to go back and explain which problem I intended to address with this
patch. Maybe you know a much better solution.

If local alloc is confronted with the following rtl it assignes pseudo 83 and 84
to the same hard register.

c.22.lreg:

;; Register 83 in 8.
;; Register 84 in 8.


(insn 43 42 46 0 (set (reg:TI 83)
        (ior:TI (ashift:TI (zero_extend:TI (mod:DI (reg:DI 79)
                        (reg:DI 47 [ D.1278 ])))
                (const_int 64 [0x40]))
            (zero_extend:TI (div:DI (reg:DI 79)
                    (reg:DI 47 [ D.1278 ]))))) 205 {divmodtidi3} (insn_list:REG_DEP_TRUE 42 (insn_li
st:REG_DEP_TRUE 37 (nil)))
    (expr_list:REG_DEAD (reg:DI 79)
        (nil)))

(insn 46 43 47 0 (set (reg:DI 84)
        (ashift:DI (subreg:DI (reg:TI 83) 0)
            (const_int 2 [0x2]))) 295 {*ashldi3_64} (insn_list:REG_DEP_TRUE 43 (nil))
    (expr_list:REG_DEAD (reg:TI 83)
        (nil)))


But on WORDS_BIG_ENDIAN machines the (subreg:DI (reg:TI 83) 0) refers to reg 9 
if (reg:TI 83) is assigned to register 8 as reload correctly notices:

c.23.greg:

Reloads for insn # 43
Reload 0: reload_in (DI) = (reg:DI 1 %r1 [79])
        reload_out (TI) = (reg:TI 83)
        GENERAL_REGS, RELOAD_OTHER (opnum = 0)
        reload_in_reg: (reg:DI 1 %r1 [79])
        reload_out_reg: (reg:TI 83)
        reload_reg_rtx: (reg:TI 8 %r8)

Reloads for insn # 46
Reload 0: reload_in (DI) = (subreg:DI (reg:TI 83) 0)
        GENERAL_REGS, RELOAD_FOR_INPUT (opnum = 1)
        reload_in_reg: (subreg:DI (reg:TI 83) 0)
        reload_reg_rtx: (reg:DI 9 %r9)

(insn 43 96 97 0 (set (reg:TI 8 %r8)
        (ior:TI (ashift:TI (zero_extend:TI (mod:DI (reg:DI 9 %r9)
                        (reg:DI 0 %r0 [orig:47 D.1278 ] [47])))
                (const_int 64 [0x40]))
            (zero_extend:TI (div:DI (reg:DI 9 %r9)
                    (reg:DI 0 %r0 [orig:47 D.1278 ] [47]))))) 205 {divmodtidi3} (insn_list:REG_DEP_T
RUE 42 (insn_list:REG_DEP_TRUE 37 (nil)))
    (nil))

(insn 97 43 98 0 (set (mem:TI (plus:DI (reg/f:DI 15 %r15)
                (const_int 160 [0xa0])) [6 S16 A8])
        (reg:TI 8 %r8)) 40 {movti} (nil)
    (nil))

(insn 98 97 46 0 (set (reg:DI 9 %r9)
        (mem:DI (plus:DI (reg/f:DI 15 %r15)
                (const_int 160 [0xa0])) [6 S8 A8])) 42 {*movdi_64} (nil)
    (nil))

(insn 46 98 47 0 (set (reg:DI 8 %r8 [84])
        (ashift:DI (reg:DI 9 %r9)
            (const_int 2 [0x2]))) 295 {*ashldi3_64} (insn_list:REG_DEP_TRUE 43 (nil))
    (nil))


reload recognizes this as a problem and issues two reload insns resulting in horrible
code (especially the store and load to the same stack slot directly following each 
other hurts us here):

dsgr    %r8,%r0               divide
stmg    %r8,%r9,160(%r15)     store r8 and r9 to stack
lg      %r9,160(%r15)         load the r8 slot into r9
sllg    %r4,%r9,2             shift left
ng      %r4,.L4-.L3(%r13)     and



The first chunk of my patch is triggered if:
 - we are on a WORDS_BIG_ENDIAN machine
 - we are dealing with integer modes because WORDS_BIG_ENDIAN does not concern
   floating point stuff
 - we are about to tie registers of different mode sizes together
 - and at least on of them is bigger than word mode which means that more than
   one hard reg would be necessary

The local variable offset in that function is used to deal with subregs and is
later added to reg_offset. The patch manipulates this variable before any other
calculations are done in order take care of the endianess issue. 
Regarding the offset tying a TImode register to a DImode quantity is like tying
a (subreg:DI (reg:TI ..) 0) to the quantity if we are on a words little endian
machine that's why in this case there is no addon necessary. But for 
WORDS_BIG_ENDIAN it is like tying a (subreg:DI (reg:TI ..) 8) to the quantity and
therefore the new code would add 1 to the offset (offset counts words).

The checks I've removed:

-       /* Do not combine registers unless one fits within the other.  */
-       || (offset > 0 && usize + offset > ssize)
-       || (offset < 0 && usize + offset < ssize)

Even with these checks it was possible to tie registers of different sizes
together. So I guess they never did what was described in the comment. But
with my changes on big endian machines registers of different sizes can't
be tied together anymore.

offset: The number of words used as subreg offset.
        offset < 0 subreg on the setreg side
        offset > 0 subreg on the usedreg side

ssize, usize: The size of the setreg/usedreg in words with all subregs 
              already stripped.

(32bit little endian)
e.g.: setreg:  (reg:SI 1)
      usedreg: (subreg:SI (reg:DI 5) 4)

offset = 1 usize = 2 ssize = 1 check one would prevent the regs from being tied
together

the same for setreg subregs:
e.g.: setreg:  (subreg:SI (reg:DI 1) 4)
      usedreg: (reg:SI 5)

offset = -1 usize = 1 ssize = 2

Btw. that would happen the same way for paradoxical subregs.

But why are all this subreg offset calculations done if we reject combining
registers as soon as a subreg is involved?
In case I haven't misunderstood the intention of these checks they
seem bogus to me. And because they circumvent an important optimization for
S/390 I've simply taken them out in hope that somebody would tell me if I was
wrong.

> It seems to me that it's not safe, because they aren't born or killed  
> at the same places.

The removal of these checks is certainly arguable.
I think removing them is safe because I didn't found any situation when they
could prevent wrong things. But maybe I missed something.


> Firstly, please put a comment above the big chunk of code you added,

...

> It clearly needs a comment.

I agree. I will the comments as you suggested in the final version.

Bye,

-Andreas-