| Message ID | 87pq21umqp.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
On Sun, Dec 23, 2012 at 1:43 AM, Richard Sandiford <rdsandiford@googlemail.com> 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 +0000 > +++ gcc/expr.h 2012-12-23 09:32:03.487440220 +0000 > @@ -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 +0000 > +++ gcc/expr.c 2012-12-23 09:32:03.485440208 +0000 > @@ -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 +0000 > +++ gcc/builtins.c 2012-12-23 09:34:47.813323158 +0000 > @@ -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); > } >
On Sun, Dec 23, 2012 at 10:43 AM, Richard Sandiford <rdsandiford@googlemail.com> 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 +0000 > +++ gcc/expr.h 2012-12-23 09:32:03.487440220 +0000 > @@ -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 +0000 > +++ gcc/expr.c 2012-12-23 09:32:03.485440208 +0000 > @@ -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 +0000 > +++ gcc/builtins.c 2012-12-23 09:34:47.813323158 +0000 > @@ -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); > } >
Richard Biener <richard.guenther@gmail.com> writes: > On Sun, Dec 23, 2012 at 10:43 AM, Richard Sandiford > <rdsandiford@googlemail.com> 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
On Wed, Jan 2, 2013 at 5:36 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Richard Biener <richard.guenther@gmail.com> writes: >> On Sun, Dec 23, 2012 at 10:43 AM, Richard Sandiford >> <rdsandiford@googlemail.com> 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
Richard Biener <richard.guenther@gmail.com> writes: > On Wed, Jan 2, 2013 at 5:36 PM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> Richard Biener <richard.guenther@gmail.com> writes: >>> On Sun, Dec 23, 2012 at 10:43 AM, Richard Sandiford >>> <rdsandiford@googlemail.com> 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
On Thu, Jan 3, 2013 at 7:42 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Richard Biener <richard.guenther@gmail.com> writes: >> On Wed, Jan 2, 2013 at 5:36 PM, Richard Sandiford >> <rdsandiford@googlemail.com> wrote: >>> Richard Biener <richard.guenther@gmail.com> writes: >>>> On Sun, Dec 23, 2012 at 10:43 AM, Richard Sandiford >>>> <rdsandiford@googlemail.com> 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
> 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.
On Fri, Jan 4, 2013 at 7:57 PM, Eric Botcazou <ebotcazou@adacore.com> 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
Richard Biener <richard.guenther@gmail.com> writes: > On Fri, Jan 4, 2013 at 7:57 PM, Eric Botcazou <ebotcazou@adacore.com> 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
Index: gcc/expr.h =================================================================== --- gcc/expr.h 2012-12-23 09:21:21.969086853 +0000 +++ gcc/expr.h 2012-12-23 09:32:03.487440220 +0000 @@ -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 +0000 +++ gcc/expr.c 2012-12-23 09:32:03.485440208 +0000 @@ -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 +0000 +++ gcc/builtins.c 2012-12-23 09:34:47.813323158 +0000 @@ -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); }