From patchwork Wed Jan 15 19:27:20 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 311249 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 163762C0097 for ; Thu, 16 Jan 2014 06:27:35 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:subject:date:message-id:mime-version:content-type; q=dns; s= default; b=xJ640gSZhM/biHlCY5Q+oN/Tsoa5BX5HxUjs+tK5xY0lCt0RRCxgq /Cidy5T6bOdpj5S6NIDoe/N91c5zEl3M0s/7b7sGLC6ipVwwDASF3dZsykD474UR cyp9vGgjUOxhr9Fw9SzyJBSVErg9DN8caPOVG+jbY8llaq6eRPOpbQ= 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:from :to:subject:date:message-id:mime-version:content-type; s= default; bh=725l6cGb1+62VTV1Xpb86Ncx2zk=; b=QQ0H+PH6LIhxR0hdg6zX VNSMD7lHGbGI66KKZcM9RyKM4HbVJpSvsjQOP9abh740JlJt3kVVYznZbI0SPz3p z0cEkWJUNtmGDdwAILyqckwImfeV2PowY4QvV3U/RdRwsM/e9h0nceXQRZ2fnh4i H9OkqgJcKczyuAZmY13DZK4= Received: (qmail 30971 invoked by alias); 15 Jan 2014 19:27:26 -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 30924 invoked by uid 89); 15 Jan 2014 19:27:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-we0-f175.google.com Received: from mail-we0-f175.google.com (HELO mail-we0-f175.google.com) (74.125.82.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 15 Jan 2014 19:27:25 +0000 Received: by mail-we0-f175.google.com with SMTP id p61so2208312wes.6 for ; Wed, 15 Jan 2014 11:27:22 -0800 (PST) X-Received: by 10.181.13.112 with SMTP id ex16mr4053871wid.23.1389814042271; Wed, 15 Jan 2014 11:27:22 -0800 (PST) Received: from localhost ([2.28.234.162]) by mx.google.com with ESMTPSA id q5sm8111452wia.2.2014.01.15.11.27.21 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 15 Jan 2014 11:27:21 -0800 (PST) From: Richard Sandiford To: gcc-patches@gcc.gnu.org Mail-Followup-To: gcc-patches@gcc.gnu.org, rdsandiford@googlemail.com Subject: Fix a dbr_schedule vs. delete_related_insns liveness bug Date: Wed, 15 Jan 2014 19:27:20 +0000 Message-ID: <87eh49nizr.fsf@talisman.default> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Two patches in a week about dbr_schedule and redundant insns. This one fixes a miscompilation of libgcov-driver.c for n32 and n64 MIPS. It's independent of (and predates) the 59137 patch. We had: barrier L1: C: $r1 = ... D: $r2 = ... ... A: if ... goto L1 B: if ... goto L1 where these are the only two uses of L1. First we filled A's delay slot with C: barrier L1: C: $r1 = ... L2: D: $r2 = ... ... A: if ... goto L2 C': $r1 = ... B: if ... goto L1 Now B is the only user of L1 so it "owns" the target thread. When looking for a delay slot, we realise that C is redundant with C', so delete it and go on to D. We call update_block to record the old C as a (use (insn)): barrier L1: C'': (use ($r1 = ...)) L2: D: $r2 = ... L3: ... A: if ... goto L2 C': $r1 = ... B: if ... goto L3 D': $r2 = ... So far so good. The problem is that redirecting B to L3 makes L1 unreachable, so reorg_redirect_jump=>redirect_jump (..., 1)=>delete_related_insns deletes it. Then C'' is unreachable and d_r_i deletes that too: barrier L2: D: $r2 = ... L3: ... A: if ... goto L2 C': $r1 = ... B: if ... goto L3 D': $r2 = ... So now, when we try to calculate the liveness beyond D, we go back to the barrier but have no indication that $r1 is live. I wondered about several ways of fixing this. * Back in the day, we just deleted the label and not related instructions. I don't think that's something we should return to though. If we delete all uses of: barrier L1: insn goto L2 to get: barrier L1: (use (insn)) goto L2 then we should definitely remove the branch to L2. * Deferring deleting labels until the end of dbr_schedule. On its own this would pessimise the rest of dbr_schedule, e.g. a jump only "owns" a thread until the next label. We would need to spinkle in a few checks for the usage count being nonzero. * Deferring deleting labels that fit the problem pattern. Having two forms of redirect would make things even more complicated though. And I don't think it would be robust in cases like the "goto L2" above. If we delete a "goto L2" that is the last use of L2, we delete L2 too, which in turn could be a label-after-barrier. So reorg would have to predict what the recursive calls to delete_related_insns would do. * Add the liveness information to the basic block info and let the (use (insn))s be deleted. I started down that path but it soon got very convoluted. Also: We used to try to update the live status of registers if WHERE is at the start of a basic block, but that can't work since we may remove a BARRIER in relax_delay_slots. */ suggests that this has already been tried and it wasn't robust. So in the end I just taught delete_related_insns to keep (use (insn)), which AFAIK is a pattern that only dbr_schedule could generate. Tested on mips64-linux-gnu. OK to install? Thanks, Richard gcc/ * jump.c (delete_related_insns): Keep (use (insn))s. * reorg.c (redundant_insn): Check for barriers too. Index: gcc/jump.c =================================================================== --- gcc/jump.c 2014-01-14 14:40:05.179783568 +0000 +++ gcc/jump.c 2014-01-14 19:10:18.392192242 +0000 @@ -1355,6 +1355,13 @@ delete_related_insns (rtx insn) /* Keep going past other deleted labels to delete what follows. */ else if (code == CODE_LABEL && INSN_DELETED_P (next)) next = NEXT_INSN (next); + /* Keep the (use (insn))s created by dbr_schedule, which needs + them in order to track liveness relative to a previous + barrier. */ + else if (INSN_P (next) + && GET_CODE (PATTERN (next)) == USE + && INSN_P (XEXP (PATTERN (next), 0))) + next = NEXT_INSN (next); else if (code == BARRIER || INSN_P (next)) /* Note: if this deletes a jump, it can cause more deletion of unreachable code, after a different label. Index: gcc/reorg.c =================================================================== --- gcc/reorg.c 2014-01-14 14:40:05.179783568 +0000 +++ gcc/reorg.c 2014-01-14 19:11:49.487961664 +0000 @@ -1512,7 +1512,10 @@ redundant_insn (rtx insn, rtx target, rt trial && insns_to_search > 0; trial = PREV_INSN (trial)) { - if (LABEL_P (trial)) + /* (use (insn))s can come immediately after a barrier if the + label that used to precede them has been deleted as dead. + See delete_related_insns. */ + if (LABEL_P (trial) || BARRIER_P (trial)) return 0; if (!INSN_P (trial))