On Thu, Jan 3, 2013 at 7:42 PM, Richard Sandiford <rdsandif...@googlemail.com> wrote: > Richard Biener <richard.guent...@gmail.com> writes: >> On Wed, Jan 2, 2013 at 5:36 PM, Richard Sandiford >> <rdsandif...@googlemail.com> wrote: >>> Richard Biener <richard.guent...@gmail.com> writes: >>>> On Sun, Dec 23, 2012 at 10:43 AM, Richard Sandiford >>>> <rdsandif...@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
