This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] fix target/44606, reload bug on SPE
- From: Eric Botcazou <ebotcazou at adacore dot com>
- To: Nathan Froyd <froydnj at codesourcery dot com>
- Cc: gcc-patches at gcc dot gnu dot org, bernds at codesourcery dot com
- Date: Fri, 1 Oct 2010 00:16:50 +0200
- Subject: Re: [PATCH] fix target/44606, reload bug on SPE
- References: <20100930182927.GL32503@codesourcery.com>
> PR44606 is bug because GCC tries to be very clever. Before register
> allocation, we have the insns:
>
> (insn 323 319 84 6 pr44606.c:34 (set (reg:DF 750)
> (mem/u/c/i:DF (reg/f:SI 719) [5 S8 A64])) 1503 {*movdf_e500_double}
> (expr_list:REG_DEAD (reg/f:SI 719) (expr_list:REG_EQUAL (const_double:DF
> 5.0e-1 [0x0.8p+0])
> (nil))))
>
> ...
>
> (insn 63 502 491 6 pr44606.c:20 (set (reg/v:SI 221 [ x ])
> (const_int 0 [0x0])) 349 {*movsi_internal1} (nil))
>
> During register allocation, pseudo 750 gets assigned to r27. Insn 63
> receives an output reload, and choose_reload_regs goes looking for an
> possibly equivalent register that holds 0. find_equiv_regs has this
>
> chunk of code when looking for an equivalence:
> || (goal_const && REG_NOTES (p) != 0
>
> && (tem = find_reg_note (p, REG_EQUIV, NULL_RTX))
> && ((rtx_equal_p (XEXP (tem, 0), goal)
> && (valueno
> = true_regnum (valtry = SET_DEST (pat))) >= 0)
>
> || (REG_P (SET_DEST (pat))
>
> && GET_CODE (XEXP (tem, 0)) == CONST_DOUBLE
> && SCALAR_FLOAT_MODE_P (GET_MODE (XEXP (tem, 0)))
> && CONST_INT_P (goal)
> && 0 != (goaltry
> = operand_subword (XEXP (tem, 0), 0, 0,
> VOIDmode))
> && rtx_equal_p (goal, goaltry)
> && (valtry
> = operand_subword (SET_DEST (pat), 0, 0,
> VOIDmode))
> && (valueno = true_regnum (valtry)) >= 0)))
>
> || (goal_const && (tem = find_reg_note (p, REG_EQUIV,
>
> NULL_RTX))
> && REG_P (SET_DEST (pat))
> && GET_CODE (XEXP (tem, 0)) == CONST_DOUBLE
> && SCALAR_FLOAT_MODE_P (GET_MODE (XEXP (tem, 0)))
> && CONST_INT_P (goal)
> && 0 != (goaltry = operand_subword (XEXP (tem, 0), 1, 0,
> VOIDmode))
> && rtx_equal_p (goal, goaltry)
> && (valtry
> = operand_subword (SET_DEST (pat), 1, 0, VOIDmode))
> && (valueno = true_regnum (valtry)) >= 0)))
>
> The complexity here comes is because we also consider the case where we
> have a CONST_DOUBLE somewhere whose low or high part is equal to the
> value we're trying to find an equivalence for. In this case, the low
> bits of 0.5 are equal to zero, so find_equiv_regs determines that r27 is
> a suitable equivalence. After register allocation, then, we have:
>
> (insn 323 531 530 6 pr44606.c:34 (set (reg:DF 27 27 [750])
> (mem/u/c/i:DF (reg/f:SI 11 11 [719]) [5 S8 A64])) 1503
> {*movdf_e500_double} (expr_list:REG_EQUIV (const_double:DF 5.0e-1
> [0x0.8p+0]) (nil)))
>
> ...
>
> (insn 63 547 598 6 ../pr44606.c:20 (set (reg:SI 27 27 [orig:750+4 ] [750])
> (const_int 0 [0x0])) 349 {*movsi_internal1} (expr_list:REG_EQUAL
> (const_int 0 [0x0]) (nil)))
>
> and a later DCE determines that insn 323 is no longer needed, deleting
> it and resulting in incorrect code at runtime.
What's incorrect exactly?
> Bootstrapping in progress on x86-64. OK to commit and backport to 4.5
> and 4.4?
>
> -Nathan
>
> gcc/
> * reload1.c (emit_reload_insns): Adjust prototype. Check for
> inherited output reloads.
The return value must be documented.
> (reload_as_needed): Delete insn if emit_reload_insns returns
> true.
> (choose_reload_regs): Save equiv in reload_override_in for
> output reloads. Set reg_rtx from reload_override_in.
I think that deleting new insns in reload is too risky on the branches. Can't
we tighten the above condition instead on the branches so that it will return
false in this case?
--
Eric Botcazou