Re: Invalid reload inheritance with paradoxical subregs

Re: Invalid reload inheritance with paradoxical subregs

[Joern Rennecke]

I suggest you first find out more what is exactly reloaded and where
the inheritance occurs - inheritance can be done by choose_reload_regs
or later in emit_reload_insns and its subfunctions.

I.e. set a breakpoint on find_reloads and make it conditional on
insn->u.fld[0].rt_int == 121 && replace
, let the compilation break there, finish, and call debug_reload()
to look at the scheduled reloads.
Then set a breakpoint on choose_reload_regs, make it conditional on
chain->insn->u.fld[0].rt_int == 121
, continue to the breakpoint, finish, and call debug_reload() again.
This should tell you what MEM is actually reloaded, and if the inheritance
already happens there; depending on what you find, you have to look further
in choose_reload_regs or in the reload emitting code. 
This e-mail was sent from a group e-mail system of ARC International Plc. Full details of the registered names and addresses of companies within the ARC group can be found on the ARC website.ARC International plc, Registered Office: Verulam Point, Station WaySt. Albans AL1 5HE United Kingdom Registered in England and Wales No.  3592130savm-exch03 
 
 
 
 
 
 
 

Re: Invalid reload inheritance with paradoxical subregs

[Uros Bizjak]

On Wed, Apr 1, 2009 at 3:34 PM, Joern Rennecke <joernr@arc.com> wrote:

> I suggest you first find out more what is exactly reloaded and where
> the inheritance occurs - inheritance can be done by choose_reload_regs
> or later in emit_reload_insns and its subfunctions.
>
> I.e. set a breakpoint on find_reloads and make it conditional on
> insn->u.fld[0].rt_int == 121 && replace
> , let the compilation break there, finish, and call debug_reload()
> to look at the scheduled reloads.
> Then set a breakpoint on choose_reload_regs, make it conditional on
> chain->insn->u.fld[0].rt_int == 121
> , continue to the breakpoint, finish, and call debug_reload() again.
> This should tell you what MEM is actually reloaded, and if the inheritance
> already happens there; depending on what you find, you have to look further
> in choose_reload_regs or in the reload emitting code.

Joern,

thans for your detailed instructions. I think I found, where problem is:

Combine simplifies lshiftrt/shift/and combined instruction under the
assumption, that for ZERO_EXTEND LOAD_EXTEND_OP targets it can prove
that certain bits are zero, so AND operation can be omitted. The
resulting instruction is valid only for memory operand:

(gdb) p debug_rtx (insn)
(insn 121 120 125 2 t.c:22 (set (reg:SI 6 r6 [orig:48 y$k.115 ] [48])
        (lshiftrt:SI (subreg:SI (mem/s/c:HI (plus:SI (reg/f:SI 1 r1)
                        (const_int 16 [0x10])) [0+4 S2 A32]) 0)
            (const_int 5 [0x5]))) 62 {lshrsi3} (nil))

Now, tracing this instruction through reload_as_needed () function, we
have following reloads just after the call to choose_reload_regs
(...):

(gdb) p debug_reload ()
Reload 0: reload_in (HI) = (mem/s/c:HI (plus:SI (reg/f:SI 1 r1)
                                                        (const_int 16
[0x10])) [0+4 S2 A32])
	GENERAL_REGS, RELOAD_FOR_INPUT (opnum = 1)
	reload_in_reg: (mem/s/c:HI (plus:SI (reg/f:SI 1 r1)
                                                        (const_int 16
[0x10])) [0+4 S2 A32])
	reload_reg_rtx: (reg:HI 7 r7)

This is all correct, until subst_reloads (...) is called a coouple of
lines down the source. This call changes (insn 121) to:

(gdb) p debug_rtx (insn)
(insn 121 120 125 2 t.c:22 (set (reg:SI 6 r6 [orig:48 y$k.115 ] [48])
        (lshiftrt:SI (subreg:SI (reg:HI 7 r7) 0)
            (const_int 5 [0x5]))) 62 {lshrsi3} (nil))

This substitution is wrong. Paradoxical subreg  of a memory operand is
for our target known to zero extend to SImode value, however -
paradoxical subreg of a register operand has undefined bits outside
the reg. So, combine and reload doesn't agree on the bits outside the
register.

After this substitution, subreg RTX is simply stripped away via the
call to cleanup_subreg_operands () in reload () and that leaves us
with undefined bits shifted in.

I guess that somehow we have to prevent choose_reload_regs to blindly
substitute memory_operand with register when paradoxical subregs are
involved. The condition for the substitution should be similar to the
condition in nonzero_bits1 from final.c:

#if defined (WORD_REGISTER_OPERATIONS) && defined (LOAD_EXTEND_OP)
	  /* If this is a typical RISC machine, we only have to worry
	     about the way loads are extended.  */
	  if ((LOAD_EXTEND_OP (GET_MODE (SUBREG_REG (x))) == SIGN_EXTEND
	       ? (((nonzero
		    & (((unsigned HOST_WIDE_INT) 1
			<< (GET_MODE_BITSIZE (GET_MODE (SUBREG_REG (x))) - 1))))
		   != 0))
	       : LOAD_EXTEND_OP (GET_MODE (SUBREG_REG (x))) != ZERO_EXTEND)
	      || !MEM_P (SUBREG_REG (x)))
#endif

Uros.

Re: Invalid reload inheritance with paradoxical subregs

[Joern Rennecke]

On Thu, Apr 02, 2009 at 12:51:55PM +0200, Uros Bizjak wrote:
> I guess that somehow we have to prevent choose_reload_regs to blindly
> substitute memory_operand with register when paradoxical subregs are
> involved. The condition for the substitution should be similar to the
> condition in nonzero_bits1 from final.c:


Yes.  It would be nicer if we could get rid of these special meanings
of paradoxical subregs and instead used ZERO_EXTEND and SIGN_EXTEND in
their place; this would also give better code for target that can
have either of these.  However, that would not only require changes to
reload to reload these properly, but also to all the affected targets,
e.g. rtx_costs would have to discount the cost of the extension operation. 
This e-mail was sent from a group e-mail system of ARC International Plc. Full details of the registered names and addresses of companies within the ARC group can be found on the ARC website.ARC International plc, Registered Office: Verulam Point, Station WaySt. Albans AL1 5HE United Kingdom Registered in England and Wales No.  3592130savm-exch03 
 
 
 
 
 
 
 

Re: Invalid reload inheritance with paradoxical subregs

[Hans-Peter Nilsson]

On Thu, 2 Apr 2009, Uros Bizjak wrote:
> Combine simplifies lshiftrt/shift/and combined instruction under the
> assumption, that for ZERO_EXTEND LOAD_EXTEND_OP targets it can prove
> that certain bits are zero, so AND operation can be omitted. The
> resulting instruction is valid only for memory operand:
...
> (gdb) p debug_rtx (insn)
> (insn 121 120 125 2 t.c:22 (set (reg:SI 6 r6 [orig:48 y$k.115 ] [48])
>         (lshiftrt:SI (subreg:SI (reg:HI 7 r7) 0)
>             (const_int 5 [0x5]))) 62 {lshrsi3} (nil))
>
> This substitution is wrong. Paradoxical subreg  of a memory operand is
> for our target known to zero extend to SImode value, however -
> paradoxical subreg of a register operand has undefined bits outside
> the reg. So, combine and reload doesn't agree on the bits outside the
> register.

Looks like the issue I attributed to IRA for
mmix-knuth-mmixware:
<http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38603>
(no, not a great deal of analysis there, I'm afraid).

And again, I'd be perfectly fine with removing this weird
LOAD_EXTEND_OP-specific "feature".  I'm of half a mind to remove
the #define from the MMIX port.

brgds, H-P

Re: Invalid reload inheritance with paradoxical subregs

[Uros Bizjak]

Hans-Peter Nilsson wrote:
> On Thu, 2 Apr 2009, Uros Bizjak wrote:
>   
>> Combine simplifies lshiftrt/shift/and combined instruction under the
>> assumption, that for ZERO_EXTEND LOAD_EXTEND_OP targets it can prove
>> that certain bits are zero, so AND operation can be omitted. The
>> resulting instruction is valid only for memory operand:
>>     
> ...
>   
>> (gdb) p debug_rtx (insn)
>> (insn 121 120 125 2 t.c:22 (set (reg:SI 6 r6 [orig:48 y$k.115 ] [48])
>>         (lshiftrt:SI (subreg:SI (reg:HI 7 r7) 0)
>>             (const_int 5 [0x5]))) 62 {lshrsi3} (nil))
>>
>> This substitution is wrong. Paradoxical subreg  of a memory operand is
>> for our target known to zero extend to SImode value, however -
>> paradoxical subreg of a register operand has undefined bits outside
>> the reg. So, combine and reload doesn't agree on the bits outside the
>> register.
>>     
>
> Looks like the issue I attributed to IRA for
> mmix-knuth-mmixware:
> <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38603>
> (no, not a great deal of analysis there, I'm afraid).
>
> And again, I'd be perfectly fine with removing this weird
> LOAD_EXTEND_OP-specific "feature".  I'm of half a mind to remove
> the #define from the MMIX port.
>   

Please note, that my findings were on 4.3.4 to-be-released branch. I'm 
afraid that IRA has nothing to do with this problem, IMO we are looking 
at plain reload bug here.  Removing your #define won't fix the failure, 
it will simply change register allocation slightly to _hide_ the failure.

The problem is, that reload should not inherit/substitute memory operand 
(where LOAD_EXTEND_OP defines bits outside the range) with register 
operand (where bits outside the range are undefined).

Uros.

Re: Invalid reload inheritance with paradoxical subregs

[Hans-Peter Nilsson]

On Mon, 6 Apr 2009, Uros Bizjak wrote:
> Hans-Peter Nilsson wrote:
> > And again, I'd be perfectly fine with removing this weird
> > LOAD_EXTEND_OP-specific "feature".  I'm of half a mind to remove
> > the #define from the MMIX port.
> Please note, that my findings were on 4.3.4 to-be-released branch. I'm afraid
> that IRA has nothing to do with this problem, IMO we are looking at plain
> reload bug here.

IRA was just "needed" for MMIX to *uncover* the bug.

>  Removing your #define won't fix the failure, it will simply
> change register allocation slightly to _hide_ the failure.

I was referring to the MMIX #define LOAD_EXTEND_OP, not (the
never committed) #define IRA_COVER_CLASSES.

> The problem is, that reload should not inherit/substitute memory operand
> (where LOAD_EXTEND_OP defines bits outside the range) with register operand
> (where bits outside the range are undefined).

Something like that, yes.

brgds, H-P

Invalid reload inheritance with paradoxical subregs

[Uros Bizjak]

Hello!

I have encountered a problem with a private RISC target, where invalid
reload is generated when paradoxical registers are involved.

In a lreg pass, I have a sequence of instructions:

(insn 112 182 114 2 t.c:22 (set (mem/s/j/c:SI (plus:SI (reg/f:SI 35 frame)
                (const_int -20 [0xffffffec])) [0+4 S4 A32])
        (reg:SI 129)) 9 {*movsi_internal_ba1} (nil))

...

(insn 116 115 117 2 t.c:22 (set (mem/s/c:HI (plus:SI (reg/f:SI 35 frame)
                (const_int -8 [0xfffffff8])) [2 y+4 S2 A32])
        (subreg:HI (reg:SI 129) 0)) 2 {*movhi_internal_ba1}
(expr_list:REG_DEAD (reg:SI 129)
        (nil)))

...

(insn 121 120 125 2 t.c:22 (set (reg:SI 48 [ y$k.115 ])
        (lshiftrt:SI (subreg:SI (mem/s/c:HI (plus:SI (reg/f:SI 35 frame)
                        (const_int -8 [0xfffffff8])) [0+4 S2 A32]) 0)
            (const_int 5 [0x5]))) 62 {lshrsi3} (nil))


(reg 129) is stored in HImode to a HImode stack slot. Since this is
ZERO_EXTEND LOAD_EXTEND_OP RISC target with WORD_REGISTER_OPERATIONS,
we zero extend the value when the value is loaded from memory in (insn
121). Effectively, we clear top 16 bits before we shift them.

After reload, this same sequence reads as:

(insn 112 182 114 2 t.c:22 (set (mem/s/j/c:SI (plus:SI (reg/f:SI 1 r1)
                (const_int 4 [0x4])) [0+4 S4 A32])
        (reg:SI 7 r7 [129])) 9 {*movsi_internal_ba1} (nil))

...

(insn 116 115 117 2 t.c:22 (set (mem/s/c:HI (plus:SI (reg/f:SI 1 r1)
                (const_int 16 [0x10])) [2 y+4 S2 A32])
        (reg:HI 7 r7 [129])) 2 {*movhi_internal_ba1} (nil))

...

(insn 121 120 125 2 t.c:22 (set (reg:SI 6 r6 [orig:48 y$k.115 ] [48])
        (lshiftrt:SI (reg:SI 7 r7)
            (const_int 5 [0x5]))) 62 {lshrsi3} (nil))


A SImode value is stored in (insn 112) and a HImode lowpart value is
stored from the same register to different value in (insn 116).
However, in (insn 121) a HImode store/load sequence that would clear
top 16 bits is simply replaced with a SImode register access. Since
the top 16 bits are _not_ guaranteed to be zero in (insn 121),
undefined bits are shifted in, leading to the wrong value in r6.

I would like to fix this problem, so if anybody (a reload expert
perhaps ;) has any advice where to look in reload1.c, I'd be more than
happy to debug and test the fix for this problem.

Thanks,
Uros.