[PATCH] Fix register corruption bug in ree

Jeff Law law@redhat.com
Fri Sep 5 19:14:00 GMT 2014


On 09/04/14 08:05, Wilco Dijkstra wrote:
> Hi,
>
> While changing register costs for AArch64 (patches to follow), the test
> gcc.target/aarch64/vect-mult.c fails. This is caused by ree inserting a TI mode copy of a DI
> register. Since TI mode requires 2 registers, this results in silent corruption of the 2nd register.
>
> After split2:
>
> (insn 149 148 147 2 (set (reg:DI 3 x3 [90])
>          (reg:DI 2 x2 [90])) vect-mull.c:78 34 {*movdi_aarch64}
>       (nil))
> (insn 152 127 153 2 (set (reg:TI 32 v0 [90])
>          (zero_extend:TI (reg:DI 3 x3 [90]))) vect-mull.c:78 719 {aarch64_movtilow_di}
>       (nil))
>
> Ree transforms this into:
>
> (insn 149 148 157 2 (set (reg:TI 32 v0)
>          (zero_extend:TI (reg:DI 2 x2 [90]))) vect-mull.c:78 719 {aarch64_movtilow_di}
>       (nil))
> (insn 157 149 147 2 (set (reg:TI 3 x3)
>          (reg:TI 32 v0)) vect-mull.c:78 -1
>       (nil))
>
> The TI mode in the second instruction means both x3 and x4 are assigned after expansion in split4
> (rather than just x3 in the original instruction), corrupting x4. The fix is to ensure the inserted
> copy will set only one register. Also ensure we always call get_extended_src_reg() rather than
> assume there is always a single extend.
>
> OK for commit?
>
> Wilco
>
> ChangeLog:
> 2014-09-04  Wilco Dijkstra  <wdijkstr@arm.com>
>
> 	* gcc/ree.c (combine_reaching_defs):
> 	Ensure inserted copy writes a single register.
Thanks!  Jakub noticed a potential problem in this area a while back, 
but I never came up with any code to trigger and have kept that issue on 
my todo list ever since.

Rather than ensuring the inserted copy write a single register, it seems 
to me we're better off ensuring that the number of hard registers hasn't 
changed.  Obviously that's not much of an issue for things like aarch64, 
x86_64, but for the embedded 32 bit parts it's likely much more important.

So I think the test is something like

x = get_extended_src_reg (SET_SRC (PATTERN (cand->insn)))
if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
     != HARD_REGNO_NREGS  (REGNO (new_reg), GET_MODE (SET_DEST (*orig_set))
   return false;

Or something along those lines...

jeff



More information about the Gcc-patches mailing list