From patchwork Mon Jan 23 17:26:00 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 718706 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 3v6dWx6j6Vz9s3v for ; Tue, 24 Jan 2017 04:26:20 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="fjbJtMLm"; 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=bk9J31oywFjIqJBKsB/7FHIlw2qmHjuIJpU7bLXHPsjfSsZ929P+k LW3Z7Za+ZCV8vE7GN/tQTEk6AN8UVA2VJLLAsKkMQWxwa1t6EvGjFa1fFtLfRcJ2 iVGp0tU0E/+9ShBrpmJtMj5uCU5jrOt9IMgeVqjYi8+JBmTmr/VIxA= 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=bQXXyStQu75JKndkmLUCWP4Tto0=; b=fjbJtMLmhCZ/JYCsQqDTd3gSem5w OR3xvKrWAZ1TEdsl/Nrr6kDWDhUe393xvH/hhLFodXucLytnx73/rjyc2Hh0sonA DMFTzcO8Sh3DMoAo9BodyFwNJBBDJb3T+E/lUKy7LaqpA4o+IsRNAFVlr8hizU7D s5JFQiimWox3Roo= Received: (qmail 122908 invoked by alias); 23 Jan 2017 17:26:07 -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 122803 invoked by uid 89); 23 Jan 2017 17:26:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.1 required=5.0 tests=BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=dominating 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 ESMTP; Mon, 23 Jan 2017 17:26:04 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CBE3B7E9C3; Mon, 23 Jan 2017 17:26:04 +0000 (UTC) Received: from abulafia.quesejoda.com (ovpn-116-128.ams2.redhat.com [10.36.116.128]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v0NHQ2R3006124; Mon, 23 Jan 2017 12:26:03 -0500 Subject: Re: [PR tree-optimization/71691] Fix unswitching in presence of maybe-undef SSA_NAMEs (take 2) To: Richard Biener References: <5e79620e-8adb-7768-a34e-32fd286995f0@redhat.com> <1e426a06-270b-07fe-0e95-ef3382b614c2@redhat.com> <0ff3a68e-3e2c-c0df-8c48-b02dac40d621@redhat.com> Cc: gcc-patches , Jeff Law From: Aldy Hernandez Message-ID: <39c1ceff-5584-210c-6ebc-12e8d234cffb@redhat.com> Date: Mon, 23 Jan 2017 12:26:00 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: On 01/18/2017 10:10 AM, Richard Biener wrote: > On Fri, Jan 13, 2017 at 7:48 PM, Aldy Hernandez wrote: Hi Richard. I'd be happy to provide a patch, but could you please elaborate, as I'm afraid I'm not following. >> We could go back to my original, no caching version (with the other >> suggestions). That solves the problem quite simply, with no regressions. > > So let's go with a unswitching-local solution then. Based on > tree_may_unswitch_on: What do you mean by unswitching-local? > > /* Condition must be invariant. */ > FOR_EACH_SSA_TREE_OPERAND (use, stmt, iter, SSA_OP_USE) > { > /* Unswitching on undefined values would introduce undefined > behavior that the original program might never exercise. */ > if (ssa_undefined_value_p (use, true)) > return NULL_TREE; > def = SSA_NAME_DEF_STMT (use); > def_bb = gimple_bb (def); > if (def_bb > && flow_bb_inside_loop_p (loop, def_bb)) > return NULL_TREE; > > we only have to look for uses in blocks dominating the loop header block > (or in blocks post-dominating that loop header, but we can probably implement > that by simply including the loop header itself with a FIXME comment). Look for *uses*?? Do you mean definitions? I mean, we're trying to figure out whether we are going to unswitch on a use that is inside a the loop, not before or after. So perhaps we only care about *definitions* (SSA_NAME_DEF_STMT) dominating the loop header. > > Note that we only need to know whether a BB will be always executed when > the loop is entered -- there's "infrastructure" elsewhere that computes this > w/o the need of post-dominance. For example see fill_always_executed_in_1 > tree-ssa-loop-im.c (we can't use that 1:1 I think because we already use ->aux > via the original copy tables, but we could simplify it as we're only > interested in > the loop which preheader we put the unswitch condition on so we can use > a bitmap to record whether a block of the loop is always executed or not). I don't see any use of ->aux within loop unswitching, so perhaps no adjustment is needed? I verified this by successfully bootstrapping with: dominance info (or post-dominance info) originally was because we couldn't depend on dominance info being kept up to date between executions of tree_unswitch_single_loop(), which BTW, runs recursively. > Can you send a patch that does this maybe? Sure, when I understand what you suggest ;-). Again, thanks for your input here. Aldy diff --git a/gcc/tree-ssa-loop-unswitch.c b/gcc/tree-ssa-loop-unswitch.c index 92599fb..774d6bf 100644 --- a/gcc/tree-ssa-loop-unswitch.c +++ b/gcc/tree-ssa-loop-unswitch.c @@ -94,6 +94,14 @@ tree_ssa_unswitch_loops (void) struct loop *loop; bool changed = false; + basic_block bb; + FOR_ALL_BB_FN (bb, cfun) + if (bb->aux) + { + gcc_unreachable (); + } Furthermore, you say that we have this "infrastructure" without the need for post-dominance. But we still need dominance info. The function fill_always_execute_in_1 uses CDI_DOMINATORS both inside said function, and throughout its dependencies. I thought the point of pre-calculating