Re: [PATCH 2/2 v3][IRA,LRA] Fix PR86939, IRA incorrectly creates an interference between a pseudo register and a hard register

[Renlin Li <renlin.li@foss.arm.com>]

Hi Peter,

On 11/08/2018 03:21 PM, Peter Bergner wrote:
> On 11/8/18 4:57 AM, Renlin Li wrote:
>> I think I found the problem!
>>
>> As described in the PR, a hard register is used in
>> an pre/post modify expression. The hard register is live, but updated.
>> In this case, we should make it conflicting with all pseudos live at
>> that point.  Does it make sense?
> [snip]
>> It fixes the ICE of mis-compiled arm-linux-gnueabihf toolchain described in the
>> PR.
>>
>> I attached the patch for discussion.  I haven't give a complete test on arm or
>> any other targets, yet. (Probably need more adjusting)
> 
> Yes, this is the problem.  We see from the dump, that r2040 does not conflict with
> hard reg r1:
> 
> ;; a2040(r1597,l0) conflicts: <list of pseudo regs>
> ;;     total conflict hard regs:
> ;;     conflict hard regs:
I think you should look for axxx(r2040, ..)?

Maybe I am wrong (not an expert of RA), from what I observed, it is the LRA
makes the code more complex. It decides to split the live range and spill r2040.
It creates multiple instructions to reload it.
r2944 in LRA dump is the register which starts to go wrong. It is assigned as r1.


       Creating newreg=2944 from oldreg=2040, assigning class GENERAL_REGS to inheritance r2944
     Original reg change 2040->2944 (bb2):
  10905: r1:SI=r2944:SI
     Add inheritance<-original before:
  12868: r2944:SI=r2040:SI

The dump is the final state of LRA. I debug it with gdb, and there are some temporary steps
which is not observable in the final dump.

> 
> ...and we have the following RTL:
> 
> (insn 10905 179 199 2 (set (reg:SI 1 r1)
>          (reg/f:SI 2040)) "../../gcc/gcc/vec.h":1654 647 {*thumb2_movsi_vfp}
>       (nil))
> 
> ...
> 
> (insn 208 202 182 2 (set (mem/f/c:SI (pre_modify:SI (reg:SI 1 r1)
>                  (plus:SI (reg:SI 1 r1)
>                      (const_int 12 [0xc]))) [129 loop_nest.m_vec+0 S4 A32])
>          (reg:SI 1048)) "../../gcc/gcc/vec.h":1654 647 {*thumb2_movsi_vfp}
>       (expr_list:REG_INC (reg:SI 1 r1)
>          (nil)))
> 
> ...
> 
> (insn 191 189 192 2 (set (mem/f/c:SI (plus:SI (reg/f:SI 2040)
>                  (const_int 8 [0x8])) [367 ddrs_table+0 S4 A32])
>          (reg/f:SI 1047)) "../../gcc/gcc/tree-loop-distribution.c":2741 647 {*thumb2_movsi_vfp}
>       (nil))
> 
> So my patch caused us to (correctly) skip adding a conflict between r1 and
> r2040 due to the register copy in insn 10905.  However, they really should
> conflict as you found due to the definition of r1 in insn 208 and the fact
> we don't add one is a latent bug in LRA.  I think your patch is on the right
> track, but not totally there yet.  Imagine instead that the references to r1
> and r2040 were swapped, so instead we have:
> 
> (insn 10905 179 199 2 (set (reg:SI 2040)
>          (reg/f:SI 1 r1)) "../../gcc/gcc/vec.h":1654 647 {*thumb2_movsi_vfp}
>       (nil))
> 
> ...
> 
> (insn 208 202 182 2 (set (mem/f/c:SI (pre_modify:SI (reg:SI 2040)
>                  (plus:SI (reg:SI 2040)
>                      (const_int 12 [0xc]))) [129 loop_nest.m_vec+0 S4 A32])
>          (reg:SI 1048)) "../../gcc/gcc/vec.h":1654 647 {*thumb2_movsi_vfp}
>       (expr_list:REG_INC (reg:SI 2040)
>          (nil)))
> 
> ...
> 
> (insn 191 189 192 2 (set (mem/f/c:SI (plus:SI (reg/f:SI 1 r1)
>                  (const_int 8 [0x8])) [367 ddrs_table+0 S4 A32])
>          (reg/f:SI 1047)) "../../gcc/gcc/tree-loop-distribution.c":2741 647 {*thumb2_movsi_vfp}
>       (nil))
> 
> Even with your patch, we'd miss adding the conflict between r1 and r2040.
> Let me think about how we should solve this one.

Yes, I am not confident the patch will be the ultimate fix to the problem.

> 
> And a *BIG* thank you for tracking down the problem!!!
> 
Nop.

Regards,
Renlin
> Peter
>