From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23794 invoked by alias); 4 Jan 2013 10:56:16 -0000 Received: (qmail 23786 invoked by uid 22791); 4 Jan 2013 10:56:15 -0000 X-SWARE-Spam-Status: No, hits=-4.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,TW_FN X-Spam-Check-By: sourceware.org Received: from mail-we0-f174.google.com (HELO mail-we0-f174.google.com) (74.125.82.174) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 04 Jan 2013 10:56:08 +0000 Received: by mail-we0-f174.google.com with SMTP id x10so7648754wey.19 for ; Fri, 04 Jan 2013 02:56:07 -0800 (PST) MIME-Version: 1.0 Received: by 10.181.13.195 with SMTP id fa3mr1384424wid.8.1357296967245; Fri, 04 Jan 2013 02:56:07 -0800 (PST) Received: by 10.194.179.130 with HTTP; Fri, 4 Jan 2013 02:56:07 -0800 (PST) In-Reply-To: <877gnu6rcc.fsf@talisman.default> References: <87pq21umqp.fsf@talisman.default> <87obh77d9r.fsf@talisman.default> <877gnu6rcc.fsf@talisman.default> Date: Fri, 04 Jan 2013 10:56:00 -0000 Message-ID: Subject: Re: RFA: Fix ICE on PARALLEL returns when expand builtins From: Richard Biener To: Richard Biener , gcc-patches@gcc.gnu.org, Richard Sandiford , Eric Botcazou Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2013-01/txt/msg00174.txt.bz2 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