From patchwork Mon May 15 23:46:40 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Santos X-Patchwork-Id: 762758 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 3wRcYr3jjsz9s85 for ; Tue, 16 May 2017 09:42:03 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="VZ2O9jeE"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type:content-transfer-encoding; q=dns; s= default; b=W3LPvhEIxYHHV4CVZeMIm5RUPcBv0xDdhcKahTeWmOLSqqoagTbzp e6IocyqzcgidMKJ7e6UAE0HSLIdNeML89eK3uf5ldjCqhPCJa+LMtKxuXE2mlBxc A3cSJoklZq4SDMnkLSpNW/TbCzf811JpgN4whTR+AikQsNpNdwcQ3U= 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 :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type:content-transfer-encoding; s=default; bh=sosy6E+BxlEA6F7xGhVQLh1OlGY=; b=VZ2O9jeEonVbs6e9fvJXdlrS440X g3BCjwOuVLakCf81xIRjAOMv9+VpySJsnbQRg8/8kss0JOaBZFVJC5Ty/9kPs0bx 9iv1GvEPXelYmwpdws0dn2cYMw6gWZ0Vy0aQxLGEgXEd1LKCxOOw4dy6IkrfcVvK m+0Yd8ulPfRnY30= Received: (qmail 111522 invoked by alias); 15 May 2017 23:41:49 -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 111489 invoked by uid 89); 15 May 2017 23:41:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=highlights, incompatibility X-HELO: sasl.smtp.pobox.com Received: from pb-smtp1.pobox.com (HELO sasl.smtp.pobox.com) (64.147.108.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 15 May 2017 23:41:47 +0000 Received: from sasl.smtp.pobox.com (unknown [127.0.0.1]) by pb-smtp1.pobox.com (Postfix) with ESMTP id 7B7626CF3E; Mon, 15 May 2017 19:41:45 -0400 (EDT) Received: from pb-smtp1.nyi.icgroup.com (unknown [127.0.0.1]) by pb-smtp1.pobox.com (Postfix) with ESMTP id 7499D6CF3D; Mon, 15 May 2017 19:41:45 -0400 (EDT) Received: from [192.168.1.4] (unknown [76.215.41.237]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by pb-smtp1.pobox.com (Postfix) with ESMTPSA id BE3AA6CF3B; Mon, 15 May 2017 19:41:44 -0400 (EDT) Subject: Re: [PATCH] [i386] Recompute the frame layout less often To: Bernd Edlinger References: <9aa7427e-6cfc-b603-2ec4-1f0e02ae294d@pobox.com> <20c96fa0-328b-af1a-c558-dab6652c482e@pobox.com> Cc: "gcc-patches@gcc.gnu.org" From: Daniel Santos Message-ID: Date: Mon, 15 May 2017 18:46:40 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: X-Pobox-Relay-ID: 0E0026B8-39C8-11E7-B8B6-EFB41968708C-06139138!pb-smtp1.pobox.com X-IsSubscribed: yes On 05/15/2017 03:39 PM, Bernd Edlinger wrote: > On 05/15/17 03:39, Daniel Santos wrote: >> On 05/14/2017 11:31 AM, Bernd Edlinger wrote: >>> Hi Daniel, >>> >>> there is one thing I don't understand in your patch: >>> That is, it introduces a static value: >>> >>> /* Registers who's save & restore will be managed by stubs called from >>> pro/epilogue. */ >>> static HARD_REG_SET GTY(()) stub_managed_regs; >>> >>> This seems to be set as a side effect of ix86_compute_frame_layout, >>> and depends on the register usage of the current function. >>> But values that depend on the current function need usually be >>> attached to cfun->machine, because the passes can run in parallel >>> unless I am completely mistaken, and the stub_managed_regs may >>> therefore be computed from a different function. >>> >>> >>> Bernd. >> I should add that if you want to run faster tests just on the ms to sysv >> abi code, you can use make RUNTESTFLAGS="ms-sysv.exp" check and then if >> that succeeds run the full testsuite. >> >> Daniel > Unfortunately I encounter a serious problem when my patch is used > ontop of your patch, Yes, the test suite ran without error, but then > I tried to trigger the warning and that tripped an ICE. > The reason is that cfun->machine->call_ms2sysv can be set to true > *after* reload_completed, which can be seen using the following > patch: > > Index: i386.c > =================================================================== > --- i386.c (revision 248031) > +++ i386.c (working copy) > @@ -29320,7 +29320,10 @@ > > /* Set here, but it may get cleared later. */ > if (TARGET_CALL_MS2SYSV_XLOGUES) > + { > + gcc_assert(!reload_completed); > cfun->machine->call_ms2sysv = true; > + } > } > > if (vec_len > 1) > > > That assertion is triggered in this test case: > > cat test.c > int test() > { > __builtin_printf("test\n"); > return 0; > } > > gcc -mabi=ms -mcall-ms2sysv-xlogues -fsplit-stack -c test.c > test.c: In function 'test': > test.c:5:1: internal compiler error: in ix86_expand_call, at > config/i386/i386.c:29324 > } > ^ > 0x13390a4 ix86_expand_call(rtx_def*, rtx_def*, rtx_def*, rtx_def*, > rtx_def*, bool) > ../../gcc-trunk/gcc/config/i386/i386.c:29324 > 0x1317494 ix86_expand_split_stack_prologue() > ../../gcc-trunk/gcc/config/i386/i386.c:15920 > 0x162ba21 gen_split_stack_prologue() > ../../gcc-trunk/gcc/config/i386/i386.md:12556 > 0x12f3f30 target_gen_split_stack_prologue > ../../gcc-trunk/gcc/config/i386/i386.md:12325 > 0xb237b3 make_split_prologue_seq > ../../gcc-trunk/gcc/function.c:5822 > 0xb23a08 thread_prologue_and_epilogue_insns() > ../../gcc-trunk/gcc/function.c:5958 > 0xb24840 rest_of_handle_thread_prologue_and_epilogue > ../../gcc-trunk/gcc/function.c:6428 > 0xb248c0 execute > ../../gcc-trunk/gcc/function.c:6470 > Please submit a full bug report, > with preprocessed source if appropriate. > Please include the complete backtrace with any bug report. > See for instructions. > > > so, in ix86_expand_split_stack_prologue > we first call: > ix86_finalize_stack_realign_flags (); > ix86_compute_frame_layout (&frame); > > and later: > call_insn = ix86_expand_call (NULL_RTX, gen_rtx_MEM (QImode, fn), > GEN_INT (UNITS_PER_WORD), constm1_rtx, > pop, false); > > which changes a flag with a huge impact on the frame layout, but there > is no absolutely no way how the frame layout can change once it is > finalized. > > > Any Thoughts? > > > Bernd. Well, my intention was actually to punt on those cases, but I hadn't actually tested with -fsplit-stack. It looks like ix86_expand_split_stack_prologue calls ix86_expand_call, and I hadn't anticipated it getting called after the last call to ix86_compute_frame_layout(), which your patch has probably eliminated. In the case of -fsplit-stack, I'm testing the macro flag_split_stack which (currently) just expands to check the global flag, so this could instead be done in ix86_option_override_internal () instead, but I think it highlights a somewhat deeper problem. Rather or not m->call_ms2sysv is set determines which stack layout is used when ix86_compute_frame_layout() runs. But if we can run expand_call after the final time ix86_compute_frame_layout() then we have a problem. It looks like ix86_expand_split_stack_prologue is the only function that manually calls ix86_expand_call, but maybe it would be better to modify the test to something like this: Or even use the same incompatibility tests from ix86_compute_frame_layout (and possibly move them to a static). But I didn't get an ICE when using a build from a few days ago. I've updated to the current HEAD but I'm having problems with the test failing even though there's no errors, as if expect has some finite buffer for all of the warning spam and it kills the job. I'm running another test to try and figure that out, but I have to run, so I'll get back with you on the results. Daniel diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index a78819d6b3f..c36383f6962 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -29325,7 +29325,7 @@ ix86_expand_call (rtx retval, rtx fnaddr, rtx callarg1, } /* Set here, but it may get cleared later. */ - if (TARGET_CALL_MS2SYSV_XLOGUES) + if (TARGET_CALL_MS2SYSV_XLOGUES && !reload_completed) cfun->machine->call_ms2sysv = true; }