Re: [RFC][ARM]: Fix reload spill failure (PR 60617)

[Ramana Radhakrishnan <ramana dot gcc at googlemail dot com>]

On Mon, Jun 16, 2014 at 1:53 PM, Venkataramanan Kumar
<venkataramanan.kumar@linaro.org> wrote:
> Hi Maintainers,
>
> This patch fixes the PR 60617 that occurs when we turn on reload pass
> in thumb2 mode.
>
> It occurs for the pattern "*ior_scc_scc" that gets generated for the 3
> argument of the below function call.
>
> JIT:emitStoreInt32(dst,regT0m, (op1 == dst || op2 == dst)));
>
>
> (----snip---)
> (insn 634 633 635 27 (parallel [
>             (set (reg:SI 3 r3)
>                 (ior:SI (eq:SI (reg/v:SI 110 [ dst ]) <== This operand
> r5 is registers gets assigned
>                         (reg/v:SI 112 [ op2 ]))
>                     (eq:SI (reg/v:SI 110 [ dst ]) <== This operand
>                         (reg/v:SI 111 [ op1 ]))))
>             (clobber (reg:CC 100 cc))
>         ]) ../Source/JavaScriptCore/jit/JITArithmetic32_64.cpp:179 300
> {*ior_scc_scc
> (----snip---)
>
> The issue here is that the above pattern demands 5 registers (LO_REGS).
>
> But when we are in reload, registers r0 is used for pointer to the
> class, r1 and r2 for first and second argument. r7 is used for stack
> pointer.
>
> So we are left with r3,r4,r5 and r6. But the above patterns needs five
> LO_REGS. Hence we get spill failure when processing the last register
> operand in that pattern,
>
> In ARM port,  TARGET_LIKELY_SPILLED_CLASS is defined for Thumb-1 and
> for thumb 2 mode there is mention of using LO_REG in the comment as
> below.
>
> "Care should be taken to avoid adding thumb-2 patterns that require
> many low registers"
>
> So conservative fix is not to allow this pattern for Thumb-2 mode.

I don't have an additional solution off the top of my head and
probably need to go do some digging.

It sounds like the conservative fix but what's the impact of doing so
? Have you measured that in terms of performance or code size on a
range of benchmarks ?

>
> I allowed these pattern for Thumb2 when we have constant operands for
> comparison. That makes the target tests arm/thum2-cond-cmp-1.c to
> thum2-cond-cmp-4.c pass.

That sounds fine and fair - no trouble there.

My concern is with removing the register alternatives and loosing the
ability to trigger conditional compares on 4.9 and trunk for Thumb1
till the time the "new" conditional compare work makes it in.

Ramana

>
> Regression tested with gcc 4.9 branch since in trunk this bug is
> masked revision 209897.
>
> Please provide your suggestion on this patch
>
> regards,
> Venkat.