Re: RFA: Fix ICE on PARALLEL returns when expand builtins
Richard Biener writes: > On Fri, Jan 4, 2013 at 7:57 PM, Eric Botcazou wrote: >>> Eric, any comments? >> >> Richard's patch seems fine to me, modulo the name of the routine; I'd rather >> use maybe_emit_group_store or something along these lines. > > Ok, fine with me - you're clearly more experienced with this code. Thanks, finally applied with that change, and sorry for the delay... Richard
Re: RFA: Fix ICE on PARALLEL returns when expand builtins
On Fri, Jan 4, 2013 at 7:57 PM, Eric Botcazou wrote: >> Hmm, how would anyone else get at the "padding" info dealing with the >> parallel? This looks broken if we need access to the type :/ > > Yes, you need to access the type for return values, but that's not really new. Ick :/ >> Eric, any comments? > > Richard's patch seems fine to me, modulo the name of the routine; I'd rather > use maybe_emit_group_store or something along these lines. Ok, fine with me - you're clearly more experienced with this code. Thanks, Richard. > -- > Eric Botcazou
Re: RFA: Fix ICE on PARALLEL returns when expand builtins
> Hmm, how would anyone else get at the "padding" info dealing with the > parallel? This looks broken if we need access to the type :/ Yes, you need to access the type for return values, but that's not really new. > Eric, any comments? Richard's patch seems fine to me, modulo the name of the routine; I'd rather use maybe_emit_group_store or something along these lines. -- Eric Botcazou
Re: RFA: Fix ICE on PARALLEL returns when expand builtins
On Thu, Jan 3, 2013 at 7:42 PM, Richard Sandiford wrote: > Richard Biener writes: >> On Wed, Jan 2, 2013 at 5:36 PM, Richard Sandiford >> wrote: >>> Richard Biener writes: On Sun, Dec 23, 2012 at 10:43 AM, Richard Sandiford wrote: > Some of the maths builtins can expand to a call followed by a bit > of postprocessing. With 4.8's PARALLEL return optimisations, these > embedded calls might return a PARALLEL of pseudos, but the postprocessing > isn't prepared to deal with that. This leads to an ICE in builtins-53.c > on n32 and n64 mips64-linux-gnu. > > One fix might have been to pass an explicit register target to the > expand routines, but that target's only a hint. This patch instead > adds an avoid_group_rtx function (named after gen_group_rtx) to convert > PARALLELs to pseudos where necessary. > > I wondered whether it was really safe for expand_builtin_int_roundingfn_2 > to pass "target == const0_rtx" as the "ignore" parameter to expand_call, > given that we don't actually ignore the return value ourselves > (even if the caller does). I suppose it is safe though, > since expand_call will always return const0_rtx in that case, > and this code is dealing with integer return values. > > Tested on mips64-linux-gnu. OK to install? Or is there a better way? You didn't add a testcase so I can't check myself >>> >>> It's gcc.dg/builtins-53.c. >>> - but why isn't using force_reg enough here? I can imagine other cases than PARALLELs that are not well handled, no? >>> >>> Not sure either way TBH. Fortunately expanding your own calls seems >>> to be pretty rare. >>> >>> But yeah, having force_reg (or I suppose force_operand) do it sounds >>> good in principle. The problem is that the operation needs the type >>> tree, which the force_* routines don't have. >> >> Hm? force_reg/operand only need a mode. >> >> Index: builtins.c >> === >> *** builtins.c (revision 194787) >> --- builtins.c (working copy) >> *** expand_builtin_int_roundingfn (tree exp, >> *** 2760,2765 >> --- 2760,2766 >> >> /* Truncate the result of floating point optab to integer >>via expand_fix (). */ >> + tmp = force_reg (TYPE_MODE (TREE_TYPE (TREE_TYPE (fallback_fndecl))), >> tmp); >> target = gen_reg_rtx (mode); >> expand_fix (target, tmp, 0); > > What I mean is: force_operand doesn't convert PARALLELs of pseudos > to single pseudos, so this won't work as-is. And we can't make > force_operand do the conversion using the current emit_group_store > machinery because emit_group_store needs access to the type (in order > to work out the padding), which force_operand doesn't have. Hmm, how would anyone else get at the "padding" info dealing with the parallel? This looks broken if we need access to the type :/ Now, why's that rounding function case relevant at all? The rounding fns in question return FP mode values - I seriously doubt there is any "padding" involved (and I can't see how any target would use a multi-register passing here?!) Eh. Eric, any comments? Thanks, Richard. > Richard
Re: RFA: Fix ICE on PARALLEL returns when expand builtins
Richard Biener writes: > On Wed, Jan 2, 2013 at 5:36 PM, Richard Sandiford > wrote: >> Richard Biener writes: >>> On Sun, Dec 23, 2012 at 10:43 AM, Richard Sandiford >>> wrote: Some of the maths builtins can expand to a call followed by a bit of postprocessing. With 4.8's PARALLEL return optimisations, these embedded calls might return a PARALLEL of pseudos, but the postprocessing isn't prepared to deal with that. This leads to an ICE in builtins-53.c on n32 and n64 mips64-linux-gnu. One fix might have been to pass an explicit register target to the expand routines, but that target's only a hint. This patch instead adds an avoid_group_rtx function (named after gen_group_rtx) to convert PARALLELs to pseudos where necessary. I wondered whether it was really safe for expand_builtin_int_roundingfn_2 to pass "target == const0_rtx" as the "ignore" parameter to expand_call, given that we don't actually ignore the return value ourselves (even if the caller does). I suppose it is safe though, since expand_call will always return const0_rtx in that case, and this code is dealing with integer return values. Tested on mips64-linux-gnu. OK to install? Or is there a better way? >>> >>> You didn't add a testcase so I can't check myself >> >> It's gcc.dg/builtins-53.c. >> >>> - but why isn't using force_reg enough here? I can imagine other >>> cases than PARALLELs that are not well handled, no? >> >> Not sure either way TBH. Fortunately expanding your own calls seems >> to be pretty rare. >> >> But yeah, having force_reg (or I suppose force_operand) do it sounds >> good in principle. The problem is that the operation needs the type >> tree, which the force_* routines don't have. > > Hm? force_reg/operand only need a mode. > > Index: builtins.c > === > *** builtins.c (revision 194787) > --- builtins.c (working copy) > *** expand_builtin_int_roundingfn (tree exp, > *** 2760,2765 > --- 2760,2766 > > /* Truncate the result of floating point optab to integer >via expand_fix (). */ > + tmp = force_reg (TYPE_MODE (TREE_TYPE (TREE_TYPE (fallback_fndecl))), > tmp); > target = gen_reg_rtx (mode); > expand_fix (target, tmp, 0); What I mean is: force_operand doesn't convert PARALLELs of pseudos to single pseudos, so this won't work as-is. And we can't make force_operand do the conversion using the current emit_group_store machinery because emit_group_store needs access to the type (in order to work out the padding), which force_operand doesn't have. Richard
Re: RFA: Fix ICE on PARALLEL returns when expand builtins
On Wed, Jan 2, 2013 at 5:36 PM, Richard Sandiford wrote: > Richard Biener writes: >> On Sun, Dec 23, 2012 at 10:43 AM, Richard Sandiford >> wrote: >>> Some of the maths builtins can expand to a call followed by a bit >>> of postprocessing. With 4.8's PARALLEL return optimisations, these >>> embedded calls might return a PARALLEL of pseudos, but the postprocessing >>> isn't prepared to deal with that. This leads to an ICE in builtins-53.c >>> on n32 and n64 mips64-linux-gnu. >>> >>> One fix might have been to pass an explicit register target to the >>> expand routines, but that target's only a hint. This patch instead >>> adds an avoid_group_rtx function (named after gen_group_rtx) to convert >>> PARALLELs to pseudos where necessary. >>> >>> I wondered whether it was really safe for expand_builtin_int_roundingfn_2 >>> to pass "target == const0_rtx" as the "ignore" parameter to expand_call, >>> given that we don't actually ignore the return value ourselves >>> (even if the caller does). I suppose it is safe though, >>> since expand_call will always return const0_rtx in that case, >>> and this code is dealing with integer return values. >>> >>> Tested on mips64-linux-gnu. OK to install? Or is there a better way? >> >> You didn't add a testcase so I can't check myself > > It's gcc.dg/builtins-53.c. > >> - but why isn't using force_reg enough here? I can imagine other >> cases than PARALLELs that are not well handled, no? > > Not sure either way TBH. Fortunately expanding your own calls seems > to be pretty rare. > > But yeah, having force_reg (or I suppose force_operand) do it sounds > good in principle. The problem is that the operation needs the type > tree, which the force_* routines don't have. Hm? force_reg/operand only need a mode. I'm suggesting sth like Index: builtins.c === *** builtins.c (revision 194787) --- builtins.c (working copy) *** expand_builtin_int_roundingfn (tree exp, *** 2760,2765 --- 2760,2766 /* Truncate the result of floating point optab to integer via expand_fix (). */ + tmp = force_reg (TYPE_MODE (TREE_TYPE (TREE_TYPE (fallback_fndecl))), tmp); target = gen_reg_rtx (mode); expand_fix (target, tmp, 0); (I suppose we can even use force_reg (GET_MODE (tmp), tmp) here - it shouldn't be VOIDmode ever as its of FP type). Richard. > Richard
Re: RFA: Fix ICE on PARALLEL returns when expand builtins
Richard Biener writes: > On Sun, Dec 23, 2012 at 10:43 AM, Richard Sandiford > wrote: >> Some of the maths builtins can expand to a call followed by a bit >> of postprocessing. With 4.8's PARALLEL return optimisations, these >> embedded calls might return a PARALLEL of pseudos, but the postprocessing >> isn't prepared to deal with that. This leads to an ICE in builtins-53.c >> on n32 and n64 mips64-linux-gnu. >> >> One fix might have been to pass an explicit register target to the >> expand routines, but that target's only a hint. This patch instead >> adds an avoid_group_rtx function (named after gen_group_rtx) to convert >> PARALLELs to pseudos where necessary. >> >> I wondered whether it was really safe for expand_builtin_int_roundingfn_2 >> to pass "target == const0_rtx" as the "ignore" parameter to expand_call, >> given that we don't actually ignore the return value ourselves >> (even if the caller does). I suppose it is safe though, >> since expand_call will always return const0_rtx in that case, >> and this code is dealing with integer return values. >> >> Tested on mips64-linux-gnu. OK to install? Or is there a better way? > > You didn't add a testcase so I can't check myself It's gcc.dg/builtins-53.c. > - but why isn't using force_reg enough here? I can imagine other > cases than PARALLELs that are not well handled, no? Not sure either way TBH. Fortunately expanding your own calls seems to be pretty rare. But yeah, having force_reg (or I suppose force_operand) do it sounds good in principle. The problem is that the operation needs the type tree, which the force_* routines don't have. Richard
Re: RFA: Fix ICE on PARALLEL returns when expand builtins
On Sun, Dec 23, 2012 at 10:43 AM, Richard Sandiford
wrote:
> Some of the maths builtins can expand to a call followed by a bit
> of postprocessing. With 4.8's PARALLEL return optimisations, these
> embedded calls might return a PARALLEL of pseudos, but the postprocessing
> isn't prepared to deal with that. This leads to an ICE in builtins-53.c
> on n32 and n64 mips64-linux-gnu.
>
> One fix might have been to pass an explicit register target to the
> expand routines, but that target's only a hint. This patch instead
> adds an avoid_group_rtx function (named after gen_group_rtx) to convert
> PARALLELs to pseudos where necessary.
>
> I wondered whether it was really safe for expand_builtin_int_roundingfn_2
> to pass "target == const0_rtx" as the "ignore" parameter to expand_call,
> given that we don't actually ignore the return value ourselves
> (even if the caller does). I suppose it is safe though,
> since expand_call will always return const0_rtx in that case,
> and this code is dealing with integer return values.
>
> Tested on mips64-linux-gnu. OK to install? Or is there a better way?
You didn't add a testcase so I can't check myself - but why isn't
using force_reg enough here? I can imagine other cases than PARALLELs
that are not well handled, no?
Thanks,
Richard.
> Richard
>
>
> gcc/
> * expr.h (avoid_group_rtx): Declare.
> * expr.c (avoid_group_rtx): New function.
> * builtins.c (expand_builtin_int_roundingfn): Call it.
> (expand_builtin_int_roundingfn_2): Likewise.
>
> Index: gcc/expr.h
> ===
> --- gcc/expr.h 2012-12-23 09:21:21.969086853 +
> +++ gcc/expr.h 2012-12-23 09:32:03.487440220 +
> @@ -334,6 +334,8 @@ extern rtx emit_group_move_into_temps (r
> PARALLEL. */
> extern void emit_group_store (rtx, rtx, tree, int);
>
> +extern rtx avoid_group_rtx (rtx, tree);
> +
> /* Copy BLKmode object from a set of registers. */
> extern void copy_blkmode_from_reg (rtx, rtx, tree);
>
> Index: gcc/expr.c
> ===
> --- gcc/expr.c 2012-12-23 09:21:21.980086911 +
> +++ gcc/expr.c 2012-12-23 09:32:03.485440208 +
> @@ -2079,6 +2079,23 @@ emit_group_store (rtx orig_dst, rtx src,
> emit_move_insn (orig_dst, dst);
> }
>
> +/* Return a form of X that does not use a PARALLEL. TYPE is the type
> + of the value stored in X. */
> +
> +rtx
> +avoid_group_rtx (rtx x, tree type)
> +{
> + enum machine_mode mode = TYPE_MODE (type);
> + gcc_checking_assert (GET_MODE (x) == VOIDmode || GET_MODE (x) == mode);
> + if (GET_CODE (x) == PARALLEL)
> +{
> + rtx result = gen_reg_rtx (mode);
> + emit_group_store (result, x, type, int_size_in_bytes (type));
> + return result;
> +}
> + return x;
> +}
> +
> /* Copy a BLKmode object of TYPE out of a register SRCREG into TARGET.
>
> This is used on targets that return BLKmode values in registers. */
> Index: gcc/builtins.c
> ===
> --- gcc/builtins.c 2012-12-23 09:21:21.981086916 +
> +++ gcc/builtins.c 2012-12-23 09:34:47.813323158 +
> @@ -2757,6 +2757,7 @@ expand_builtin_int_roundingfn (tree exp,
>exp = build_call_nofold_loc (EXPR_LOCATION (exp), fallback_fndecl, 1, arg);
>
>tmp = expand_normal (exp);
> + tmp = avoid_group_rtx (tmp, TREE_TYPE (exp));
>
>/* Truncate the result of floating point optab to integer
> via expand_fix (). */
> @@ -2860,6 +2861,7 @@ expand_builtin_int_roundingfn_2 (tree ex
>fallback_fndecl, 1, arg);
>
>target = expand_call (exp, NULL_RTX, target == const0_rtx);
> + target = avoid_group_rtx (target, TREE_TYPE (exp));
>return convert_to_mode (mode, target, 0);
> }
>
Re: RFA: Fix ICE on PARALLEL returns when expand builtins
On Sun, Dec 23, 2012 at 1:43 AM, Richard Sandiford
wrote:
> Some of the maths builtins can expand to a call followed by a bit
> of postprocessing. With 4.8's PARALLEL return optimisations, these
> embedded calls might return a PARALLEL of pseudos, but the postprocessing
> isn't prepared to deal with that. This leads to an ICE in builtins-53.c
> on n32 and n64 mips64-linux-gnu.
I had filed this as PR 55114.
Thanks,
Andrew
>
> One fix might have been to pass an explicit register target to the
> expand routines, but that target's only a hint. This patch instead
> adds an avoid_group_rtx function (named after gen_group_rtx) to convert
> PARALLELs to pseudos where necessary.
>
> I wondered whether it was really safe for expand_builtin_int_roundingfn_2
> to pass "target == const0_rtx" as the "ignore" parameter to expand_call,
> given that we don't actually ignore the return value ourselves
> (even if the caller does). I suppose it is safe though,
> since expand_call will always return const0_rtx in that case,
> and this code is dealing with integer return values.
>
> Tested on mips64-linux-gnu. OK to install? Or is there a better way?
>
> Richard
>
>
> gcc/
> * expr.h (avoid_group_rtx): Declare.
> * expr.c (avoid_group_rtx): New function.
> * builtins.c (expand_builtin_int_roundingfn): Call it.
> (expand_builtin_int_roundingfn_2): Likewise.
>
> Index: gcc/expr.h
> ===
> --- gcc/expr.h 2012-12-23 09:21:21.969086853 +
> +++ gcc/expr.h 2012-12-23 09:32:03.487440220 +
> @@ -334,6 +334,8 @@ extern rtx emit_group_move_into_temps (r
> PARALLEL. */
> extern void emit_group_store (rtx, rtx, tree, int);
>
> +extern rtx avoid_group_rtx (rtx, tree);
> +
> /* Copy BLKmode object from a set of registers. */
> extern void copy_blkmode_from_reg (rtx, rtx, tree);
>
> Index: gcc/expr.c
> ===
> --- gcc/expr.c 2012-12-23 09:21:21.980086911 +
> +++ gcc/expr.c 2012-12-23 09:32:03.485440208 +
> @@ -2079,6 +2079,23 @@ emit_group_store (rtx orig_dst, rtx src,
> emit_move_insn (orig_dst, dst);
> }
>
> +/* Return a form of X that does not use a PARALLEL. TYPE is the type
> + of the value stored in X. */
> +
> +rtx
> +avoid_group_rtx (rtx x, tree type)
> +{
> + enum machine_mode mode = TYPE_MODE (type);
> + gcc_checking_assert (GET_MODE (x) == VOIDmode || GET_MODE (x) == mode);
> + if (GET_CODE (x) == PARALLEL)
> +{
> + rtx result = gen_reg_rtx (mode);
> + emit_group_store (result, x, type, int_size_in_bytes (type));
> + return result;
> +}
> + return x;
> +}
> +
> /* Copy a BLKmode object of TYPE out of a register SRCREG into TARGET.
>
> This is used on targets that return BLKmode values in registers. */
> Index: gcc/builtins.c
> ===
> --- gcc/builtins.c 2012-12-23 09:21:21.981086916 +
> +++ gcc/builtins.c 2012-12-23 09:34:47.813323158 +
> @@ -2757,6 +2757,7 @@ expand_builtin_int_roundingfn (tree exp,
>exp = build_call_nofold_loc (EXPR_LOCATION (exp), fallback_fndecl, 1, arg);
>
>tmp = expand_normal (exp);
> + tmp = avoid_group_rtx (tmp, TREE_TYPE (exp));
>
>/* Truncate the result of floating point optab to integer
> via expand_fix (). */
> @@ -2860,6 +2861,7 @@ expand_builtin_int_roundingfn_2 (tree ex
>fallback_fndecl, 1, arg);
>
>target = expand_call (exp, NULL_RTX, target == const0_rtx);
> + target = avoid_group_rtx (target, TREE_TYPE (exp));
>return convert_to_mode (mode, target, 0);
> }
>
RFA: Fix ICE on PARALLEL returns when expand builtins
Some of the maths builtins can expand to a call followed by a bit
of postprocessing. With 4.8's PARALLEL return optimisations, these
embedded calls might return a PARALLEL of pseudos, but the postprocessing
isn't prepared to deal with that. This leads to an ICE in builtins-53.c
on n32 and n64 mips64-linux-gnu.
One fix might have been to pass an explicit register target to the
expand routines, but that target's only a hint. This patch instead
adds an avoid_group_rtx function (named after gen_group_rtx) to convert
PARALLELs to pseudos where necessary.
I wondered whether it was really safe for expand_builtin_int_roundingfn_2
to pass "target == const0_rtx" as the "ignore" parameter to expand_call,
given that we don't actually ignore the return value ourselves
(even if the caller does). I suppose it is safe though,
since expand_call will always return const0_rtx in that case,
and this code is dealing with integer return values.
Tested on mips64-linux-gnu. OK to install? Or is there a better way?
Richard
gcc/
* expr.h (avoid_group_rtx): Declare.
* expr.c (avoid_group_rtx): New function.
* builtins.c (expand_builtin_int_roundingfn): Call it.
(expand_builtin_int_roundingfn_2): Likewise.
Index: gcc/expr.h
===
--- gcc/expr.h 2012-12-23 09:21:21.969086853 +
+++ gcc/expr.h 2012-12-23 09:32:03.487440220 +
@@ -334,6 +334,8 @@ extern rtx emit_group_move_into_temps (r
PARALLEL. */
extern void emit_group_store (rtx, rtx, tree, int);
+extern rtx avoid_group_rtx (rtx, tree);
+
/* Copy BLKmode object from a set of registers. */
extern void copy_blkmode_from_reg (rtx, rtx, tree);
Index: gcc/expr.c
===
--- gcc/expr.c 2012-12-23 09:21:21.980086911 +
+++ gcc/expr.c 2012-12-23 09:32:03.485440208 +
@@ -2079,6 +2079,23 @@ emit_group_store (rtx orig_dst, rtx src,
emit_move_insn (orig_dst, dst);
}
+/* Return a form of X that does not use a PARALLEL. TYPE is the type
+ of the value stored in X. */
+
+rtx
+avoid_group_rtx (rtx x, tree type)
+{
+ enum machine_mode mode = TYPE_MODE (type);
+ gcc_checking_assert (GET_MODE (x) == VOIDmode || GET_MODE (x) == mode);
+ if (GET_CODE (x) == PARALLEL)
+{
+ rtx result = gen_reg_rtx (mode);
+ emit_group_store (result, x, type, int_size_in_bytes (type));
+ return result;
+}
+ return x;
+}
+
/* Copy a BLKmode object of TYPE out of a register SRCREG into TARGET.
This is used on targets that return BLKmode values in registers. */
Index: gcc/builtins.c
===
--- gcc/builtins.c 2012-12-23 09:21:21.981086916 +
+++ gcc/builtins.c 2012-12-23 09:34:47.813323158 +
@@ -2757,6 +2757,7 @@ expand_builtin_int_roundingfn (tree exp,
exp = build_call_nofold_loc (EXPR_LOCATION (exp), fallback_fndecl, 1, arg);
tmp = expand_normal (exp);
+ tmp = avoid_group_rtx (tmp, TREE_TYPE (exp));
/* Truncate the result of floating point optab to integer
via expand_fix (). */
@@ -2860,6 +2861,7 @@ expand_builtin_int_roundingfn_2 (tree ex
fallback_fndecl, 1, arg);
target = expand_call (exp, NULL_RTX, target == const0_rtx);
+ target = avoid_group_rtx (target, TREE_TYPE (exp));
return convert_to_mode (mode, target, 0);
}
