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 <wdijk...@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
