Re: subreg pass

[Roman Zippel <zippel at linux-m68k dot org>]

Hi,

On Mon, 5 Mar 2007, Roman Zippel wrote:

> > Yes, it is in general better now to split double-word length
> > operations before reload.  It's not necessarily better to split as
> > early as possible, as that will essentially disable the RTL level loop
> > optimizations. 
> 
> I was worried about that too, since some patterns would be more 
> complicated, which may make some optimizations more difficult, e.g. where 
> I noticed it first was mulsidi3, which currently looks like this:

Ok, I played a little more with umulsidi3 (see attached patch, only the 
umulsidi3 pattern are relevant for this) and I think found a bug somwhere, 
although I'm not sure whether the subreg pass causes it or only trigggers 
it (it goes away with -fno-split-wide-types).

I removed the '%' from the constraint to test proper reloads (which may 
not be here, but probably in other cases) and used the following testcase:

unsigned long long f(void)
{
        register unsigned int a asm ("%d0");
        register unsigned int b asm ("%d1");
        asm ("# %0  %1" : "=d" (a), "=d" (b));
        return ((unsigned long long)b * (unsigned long long)a);
}

This is the situation prior to subreg2:

(insn 32 10 30 2 (parallel [
            (set (strict_low_part (subreg:SI (reg:DI 33) 4))
                (mult:SI (reg/v:SI 0 %d0 [ a ])
                    (reg/v:SI 1 %d1 [ b ])))
            (set (strict_low_part (subreg:SI (reg:DI 33) 0))
                (truncate:SI (lshiftrt:DI (mult:DI (zero_extend:DI (reg/v:SI 0 %d0 [ a ]))
                            (zero_extend:DI (reg/v:SI 1 %d1 [ b ])))
                        (const_int 32 [0x20]))))
        ]) -1 (nil)
    (expr_list:REG_DEAD (reg/v:SI 0 %d0 [ a ])
        (expr_list:REG_DEAD (reg/v:SI 1 %d1 [ b ])
            (nil))))

(insn 30 32 31 2 (set (reg:SI 0 %d0 [ <result> ])
        (subreg:SI (reg:DI 33) 0)) 31 {*m68k.md:673} (insn_list:REG_DEP_TRUE 11 (nil))
    (nil))

(insn 31 30 23 2 (set (reg:SI 1 %d1 [orig:0 <result>+4 ] [0])
        (subreg:SI (reg:DI 33) 4)) 31 {*m68k.md:673} (nil)
    (expr_list:REG_DEAD (reg:DI 33)
        (nil)))

(insn 23 31 0 2 (use (reg/i:DI 0 %d0 [ <result> ])) -1 (insn_list:REG_DEP_TRUE 30 (nil))
    (nil))


subreg2 produces this (which looks correct):

(insn 32 10 30 2 (parallel [
            (set (strict_low_part (reg:SI 38 [+4 ]))
                (mult:SI (reg/v:SI 0 %d0 [ a ])
                    (reg/v:SI 1 %d1 [ b ])))
            (set (strict_low_part (reg:SI 37))
                (truncate:SI (lshiftrt:DI (mult:DI (zero_extend:DI (reg/v:SI 0 %d0 [ a ]))
                            (zero_extend:DI (reg/v:SI 1 %d1 [ b ])))
                        (const_int 32 [0x20]))))
        ]) 181 {*umulsidi3_subreg} (nil)
    (expr_list:REG_DEAD (reg/v:SI 0 %d0 [ a ])
        (expr_list:REG_DEAD (reg/v:SI 1 %d1 [ b ])
            (nil))))

(insn 30 32 31 2 (set (reg:SI 0 %d0 [ <result> ])
        (reg:SI 37)) 31 {*m68k.md:673} (insn_list:REG_DEP_TRUE 11 (nil))
    (expr_list:REG_DEAD (reg:SI 37)
        (nil)))

(insn 31 30 23 2 (set (reg:SI 1 %d1 [orig:0 <result>+4 ] [0])
        (reg:SI 38 [+4 ])) 31 {*m68k.md:673} (nil)
    (expr_list:REG_DEAD (reg:SI 38 [+4 ])
        (nil)))

(insn 23 31 0 2 (use (reg/i:DI 0 %d0 [ <result> ])) -1 (insn_list:REG_DEP_TRUE 30 (nil))
    (nil))


Reload has now to match (reg %d0) and (reg 38) above in insn 32 and after 
pseudo register replacement it looks like this:

(insn 32 10 30 2 (parallel [
            (set (strict_low_part (reg:SI 1 %d1 [orig:38+4 ] [38]))
                (mult:SI (reg/v:SI 0 %d0 [ a ])
                    (reg/v:SI 1 %d1 [ b ])))
            (set (strict_low_part (reg:SI 0 %d0 [37]))
                (truncate:SI (lshiftrt:DI (mult:DI (zero_extend:DI (reg/v:SI 0 %d0 [ a ]))
                            (zero_extend:DI (reg/v:SI 1 %d1 [ b ])))
                        (const_int 32 [0x20]))))
        ]) 181 {*umulsidi3_subreg} (nil)
    (expr_list:REG_DEAD (reg/v:SI 0 %d0 [ a ])
        (expr_list:REG_DEAD (reg/v:SI 1 %d1 [ b ])
            (nil))))

Notice that the REG_DEAD notes are not correct anymore, reload produces 
now the following reload:

Reloads for insn # 32
Reload 0: reload_in (SI) = (reg/v:SI 0 %d0 [ a ])
        reload_out (SI) = (reg:SI 1 %d1 [orig:38+4 ] [38])
        DATA_REGS, RELOAD_OTHER (opnum = 0)
        reload_in_reg: (reg/v:SI 0 %d0 [ a ])
        reload_out_reg: (reg:SI 1 %d1 [orig:38+4 ] [38])
        reload_reg_rtx: (reg/v:SI 0 %d0 [ a ])

The problem here is it can't use %d0 as reload register (but it does it 
anyway, because it thinks it's dead), as it's used for (reg 37) and reload 
produces this:

(insn 32 10 34 2 (parallel [
            (set (strict_low_part (reg/v:SI 0 %d0 [ a ]))
                (mult:SI (reg/v:SI 0 %d0 [ a ])
                    (reg/v:SI 1 %d1 [ b ])))
            (set (strict_low_part (reg:SI 0 %d0 [37]))
                (truncate:SI (lshiftrt:DI (mult:DI (zero_extend:DI (reg/v:SI 0 %d0 [ a ]))
                            (zero_extend:DI (reg/v:SI 1 %d1 [ b ])))
                        (const_int 32 [0x20]))))
        ]) 181 {*umulsidi3_subreg} (nil)
    (nil))

%d0 is used in both set destinations and postreload dies because of it.
The question is now, who is responsible for telling reload that %d0/%d1 
are not really dead anymore after register allocation?

bye, Roman

Attachment: umulsidi3_subreg.diff
Description: Text document