From patchwork Wed Oct 1 18:57:19 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Vladimir Makarov X-Patchwork-Id: 395641 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id E15D51400E0 for ; Thu, 2 Oct 2014 04:57:54 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type:content-transfer-encoding; q=dns; s= default; b=p1G4K8k+P4ani/TOXQqKR390vu+MOwbZkI6RTUVgvXPF2YzwSxapJ gFPU8FBvMHGUdWZGzigpWuawEmMaZjT4RtgxAemPxm4qbxrMPY00C3QFf52zxcfw C8TVUvVl/98k63Rz1FNWd/RYEl0oD4Blg1klD9XEJ3HAQSMa+I0Wuo= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type:content-transfer-encoding; s=default; bh=5xZpEeln7Bgq/r5BJVHX02S9pg4=; b=SfCpe7xZOKkWctVtS+smNozCffBI I84MPnqzeujpwD3wHXgKAX4C1DsRIluHkqV/Eo8CAxDaBR/uFXV/JXuvUNv2bn1m c8tiD9M88cJYJDGGfeKV3k5yXwSLM1IfK+MgscXV0Jn8FBLm0b7/3Uf24YDoYsS6 IJjC52/NVSsk3vs= Received: (qmail 28353 invoked by alias); 1 Oct 2014 18:57:47 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 28337 invoked by uid 89); 1 Oct 2014 18:57:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.1 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 01 Oct 2014 18:57:43 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s91Ivfbn004400 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 1 Oct 2014 14:57:42 -0400 Received: from VMBP.local (vpn-52-153.rdu2.redhat.com [10.10.52.153]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s91Ivcl6022488; Wed, 1 Oct 2014 14:57:39 -0400 Message-ID: <542C4E8F.8030502@redhat.com> Date: Wed, 01 Oct 2014 14:57:19 -0400 From: Vladimir Makarov User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Thunderbird/31.1.2 MIME-Version: 1.0 To: Ilya Enkovich , Jeff Law CC: Uros Bizjak , "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH, i386, Pointer Bounds Checker 33/x] MPX ABI References: <20140919075558.GE50194@msticlxl57.ims.intel.com> <542068E1.5060504@redhat.com> <5421B591.6090705@redhat.com> <542316DC.5030700@redhat.com> In-Reply-To: X-IsSubscribed: yes On 2014-09-25 5:46 AM, Ilya Enkovich wrote: > 2014-09-25 1:51 GMT+04:00 Ilya Enkovich : >> 2014-09-24 23:09 GMT+04:00 Jeff Law : >>> On 09/24/14 07:13, Ilya Enkovich wrote: >>>> >>>> I tried to generate PARALLEL with all regs set by call. Here is a >>>> memset call I got: >>>> >>>> (call_insn 23 22 24 2 (set (parallel [ >>>> (expr_list:REG_DEP_TRUE (reg:DI 0 ax) >>>> (const_int 0 [0])) >>>> (expr_list:REG_DEP_TRUE (reg:BND64 77 bnd0) >>>> (const_int 0 [0])) >>>> (expr_list:REG_DEP_TRUE (reg:BND64 78 bnd1) >>>> (const_int 0 [0])) >>>> ]) >>>> (call/j (mem:QI (symbol_ref:DI ("memset") [flags 0x41] >>> >>> [ snip ] >>> Looks good. This is the approved way to handle multiple results of a call. >>> >>>> >>>> During register allocation LRA generated a weird move instruction: >>>> >>>> (insn 63 0 0 (set (reg/f:DI 100) >>>> (parallel [ >>>> (expr_list:REG_DEP_TRUE (reg:DI 0 ax) >>>> (const_int 0 [0])) >>>> (expr_list:REG_DEP_TRUE (reg:BND64 77 bnd0) >>>> (const_int 0 [0])) >>>> (expr_list:REG_DEP_TRUE (reg:BND64 78 bnd1) >>>> (const_int 0 [0])) >>>> ])) -1 >>>> (nil)) >>>> >>>> Which caused ICE later in LRA. This move happens because of >>>> REG_RETURNED (reg/f:DI 100) (see condition in inherit_in_ebb at >>>> lra-constraints.c:5312). Thus this code in LRA doesn't accept >>>> PARALLEL dest for calls. >>> >>> This is a bug in LRA then. Multiple return values aren't heavily used, so >>> I'm not surprised that its handling was missed in LRA. >>> >>> The question now is how to bundle things together in such a way as to make >>> it easy for Vlad to reproduce and fix this in LRA. >>> >>> Jeff >> >> I suppose it should be easy to reproduce using the same test case I >> use and some speudo patch which adds fake return values (e.g. xmm6 and >> xmm7) to calls. Will try to make some minimal patch and test Vlad >> could work with. >> >> Ilya > > I couldn't reproduce the problem on a small test but chrome build > shows a lot of errors. Due to the nature of the problem test's size > shouldn't matter, so I attach patch which emulates situation with > bounds regs (but uses xmm5 and xmm6 instead of bnd0 and bnd1) with a > preprocessed chrome file. > The problem is in code introduced by Bernd in IRA and caller-saves.c in 2012. It is basically an optimization for functions returning always the same result as one argument (e.g. memcpy returning 1st argument). There are two possible solutions. First one is to prohibit the optimizations when there is a parallel in SET. Second one is to go deeper if the call result is guaranteed in the first element which is true for the patch. For the first solution, the patch would be } The first patch is safer but the second one is ok too. I have no particular preferences. Whatever we choose, analogous code in caller-saves.c should be changed too. Index: lra-constraints.c =================================================================== --- lra-constraints.c (revision 215748) +++ lra-constraints.c (working copy) @@ -5348,16 +5348,19 @@ if (GET_CODE (pat) == PARALLEL) pat = XVECEXP (pat, 0, 0); dest = SET_DEST (pat); - start_sequence (); - emit_move_insn (cheap, copy_rtx (dest)); - restore = get_insns (); - end_sequence (); - lra_process_new_insns (curr_insn, NULL, restore, - "Inserting call parameter restore"); - /* We don't need to save/restore of the pseudo from - this call. */ - usage_insns[regno].calls_num = calls_num; - bitmap_set_bit (&check_only_regs, regno); + if (REG_P (dest)) + { + start_sequence (); + emit_move_insn (cheap, copy_rtx (dest)); + restore = get_insns (); + end_sequence (); + lra_process_new_insns (curr_insn, NULL, restore, + "Inserting call parameter restore"); + /* We don't need to save/restore of the pseudo + from this call. */ + usage_insns[regno].calls_num = calls_num; + bitmap_set_bit (&check_only_regs, regno); + } } } to_inherit_num = 0; For the second solution, the patch is Index: lra-constraints.c =================================================================== --- lra-constraints.c (revision 215748) +++ lra-constraints.c (working copy) @@ -5348,16 +5348,25 @@ if (GET_CODE (pat) == PARALLEL) pat = XVECEXP (pat, 0, 0); dest = SET_DEST (pat); - start_sequence (); - emit_move_insn (cheap, copy_rtx (dest)); - restore = get_insns (); - end_sequence (); - lra_process_new_insns (curr_insn, NULL, restore, - "Inserting call parameter restore"); - /* We don't need to save/restore of the pseudo from - this call. */ - usage_insns[regno].calls_num = calls_num; - bitmap_set_bit (&check_only_regs, regno); + if (GET_CODE (dest) == PARALLEL) + { + dest = XVECEXP (dest, 0, 0); + if (GET_CODE (dest) == EXPR_LIST) + dest = XEXP (dest, 0); + } + if (REG_P (dest)) + { + start_sequence (); + emit_move_insn (cheap, copy_rtx (dest)); + restore = get_insns (); + end_sequence (); + lra_process_new_insns (curr_insn, NULL, restore, + "Inserting call parameter restore"); + /* We don't need to save/restore of the pseudo from + this call. */ + usage_insns[regno].calls_num = calls_num; + bitmap_set_bit (&check_only_regs, regno); + } }