From patchwork Thu May 19 12:18:55 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Kugan Vivekanandarajah X-Patchwork-Id: 68124 Delivered-To: patch@linaro.org Received: by 10.140.92.199 with SMTP id b65csp3167810qge; Thu, 19 May 2016 05:19:25 -0700 (PDT) X-Received: by 10.98.75.81 with SMTP id y78mr19170366pfa.161.1463660365587; Thu, 19 May 2016 05:19:25 -0700 (PDT) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id e89si19735994pfj.100.2016.05.19.05.19.25 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 19 May 2016 05:19:25 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-return-427734-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-427734-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-427734-patch=linaro.org@gcc.gnu.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=vomYw5/m+GwrBa6gwn G3LevA2D9vfhFmUhn9HrtF3P0TSKJPltgM2yZpeztqM8bDc3BGqFkDeC7DYKfFKN 2gCWWR0GXtURVqP9qL+qw6uMKc1E4KgpTIQ0a0WO3Cxc+Mgu/BWnZRN5KtWxDjjZ ud7Acj7I59ctUq2DlFUd5w0Zs= 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 :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=i3ujRm3+aonWOrZSsJTWIeMj JHs=; b=r/Sd9RJKworClR/0LjqgFSJYnuNcCazstW+4GPvCSP+ROCy4B+nYccFm O+8HQnErcWdbLW3JsrroX0tiPzOUIcO+9TfsAitoeGg7+XsGAMFOjNUPAnZ+qhQJ 2r9247PHJWZlqTc1jH3GOnftZTk1ECh45J8azovsI6bbQC+MRW4= Received: (qmail 54227 invoked by alias); 19 May 2016 12:19:11 -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 53331 invoked by uid 89); 19 May 2016 12:19:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=ending X-HELO: mail-qg0-f44.google.com Received: from mail-qg0-f44.google.com (HELO mail-qg0-f44.google.com) (209.85.192.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 19 May 2016 12:18:58 +0000 Received: by mail-qg0-f44.google.com with SMTP id f92so41925337qgf.0 for ; Thu, 19 May 2016 05:18:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=Gt9Nr8plzjomJTK71ZKUH++nXXjjBHI0kfEVsucmLKQ=; b=kyiKcHh754HCdkce6hm0UmFJ3lF03R1Gh+/h7ldLB/zVIWy9iDkMiQmmTJk/KvhHsX qqUGMgoSyNMgNo83b37zyDeMyGSdKan2h2cpGunyrDsczW99O8WwtyAbMqXIig6YOUmM k26M8VN1BCRpt3KKCcY3IjoWRzCs1WdQXrMrm8O66HB948N2/k9zyfgSscx7U2XJwc6E VQnFU8NhLcXGDgRP5FvhRST0AcsZBGbmFeHL16FA5jURC5o4MAp/WtpMqDWfXjo+Ef11 HOWzXOl1/1WhjZLPpMYNIWGJ3FbDH8Qt6pdzc6/nBKsNiK9SWIN7AMo3DHAVrITK8vpy IkuA== X-Gm-Message-State: AOPr4FXtpn8ilVG32e+P6BM7zft++/GUjHh2Qb3PFbpFcSB8oDbaBj5wmNIt+CspohdyKOc+k5WIWolW5+QmmUiP MIME-Version: 1.0 X-Received: by 10.140.98.38 with SMTP id n35mr13001278qge.22.1463660335890; Thu, 19 May 2016 05:18:55 -0700 (PDT) Received: by 10.200.42.71 with HTTP; Thu, 19 May 2016 05:18:55 -0700 (PDT) In-Reply-To: References: <573D7394.5050208@suse.cz> <573D78CE.6020900@linaro.org> Date: Thu, 19 May 2016 22:18:55 +1000 Message-ID: Subject: Re: [PATCH] Fix PR tree-optimization/71170 From: Kugan Vivekanandarajah To: Richard Biener Cc: =?UTF-8?Q?Martin_Li=C5=A1ka?= , GCC Patches X-IsSubscribed: yes On 19 May 2016 at 18:55, Richard Biener wrote: > On Thu, May 19, 2016 at 10:26 AM, Kugan > wrote: >> Hi, >> >> >> On 19/05/16 18:21, Richard Biener wrote: >>> On Thu, May 19, 2016 at 10:12 AM, Kugan Vivekanandarajah >>> wrote: >>>> Hi Martin, >>>> >>>> Thanks for the fix. Just to elaborate (as mentioned in PR) >>>> >>>> At tree-ssa-reassoc.c:3897, we have: >>>> >>>> stmt: >>>> _15 = _4 + c_7(D); >>>> >>>> oe->op def_stmt: >>>> _17 = c_7(D) * 3; >>>> >>>> >>>> : >>>> a1_6 = s_5(D) * 2; >>>> _1 = (long int) a1_6; >>>> x1_8 = _1 + c_7(D); >>>> a2_9 = s_5(D) * 4; >>>> _2 = (long int) a2_9; >>>> a3_11 = s_5(D) * 6; >>>> _3 = (long int) a3_11; >>>> _16 = x1_8 + c_7(D); >>>> _18 = _1 + _2; >>>> _4 = _16 + _2; >>>> _15 = _4 + c_7(D); >>>> _17 = c_7(D) * 3; >>>> x_13 = _15 + _3; >>>> return x_13; >>>> >>>> >>>> The root cause of this the place in which we are adding (_17 = c_7(D) >>>> * 3). Finding the right place is not always straightforward as this >>>> case shows. >>>> >>>> We could try Martin Liška's approach, We could also move _17 = c_7(D) >>>> * 3; at tree-ssa-reassoc.c:3897 satisfy the gcc_assert. We could do >>>> this based on the use count of _17. >>>> >>>> >>>> This patch does this. I have no preferences. Any thoughts ? >>> >>> I think the issue may be that you fail to set changed to true for the >>> degenerate case of ending up with a multiply only. >>> >>> Not sure because neither patch contains a testcase. >>> >> >> Sorry, I should have been specific. There is an existing test-case that >> is failing. Thats why I didn't include a test case. >> >> FAIL: gcc.dg/tree-ssa/slsr-30.c (internal compiler error) > > Btw, it also looks like ops are not sorted after rank: > > (gdb) p ops.m_vec->m_vecdata[0] > $4 = (operand_entry *) 0x27a82e0 > (gdb) p ops.m_vec->m_vecdata[1] > $5 = (operand_entry *) 0x27a82a0 > (gdb) p ops.m_vec->m_vecdata[2] > $6 = (operand_entry *) 0x27a8260 > (gdb) p ops.m_vec->m_vecdata[3] > $7 = (operand_entry *) 0x27a8300 > (gdb) p *$4 > $8 = {rank = 7, id = 5, op = , count = 1} > (gdb) p *$5 > $9 = {rank = 5, id = 3, op = , count = 1} > (gdb) p *$6 > $10 = {rank = 7, id = 1, op = , count = 1} > (gdb) p *$7 > $11 = {rank = 7, id = 6, op = , count = 1} > Hi Richard, It is sorted but in swap_ops_for_binary_stmt (ops, len - 3, stmt), it is reordered for some form optimization. Commenting this is not helping the ICE. In the ops list, the operand defined by multiplication has to be the last due to the way we add the multiplication stmt. We don’t have the knowledge to find the optimal point without going through some complicated logic. Therefore, I think we either should reorder the stmt (like my previous patch) Or change the rank of the operand produced by the multiply such that it will always be the last. This looks like a reasonable think to do. Please let me know what you think. I am attaching the patch (not tested). I will do the proper test and report the results if you think this approach is OK. Thanks, Kugan diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c index 3b5f36b..52414d3 100644 --- a/gcc/tree-ssa-reassoc.c +++ b/gcc/tree-ssa-reassoc.c @@ -553,12 +553,12 @@ sort_by_operand_rank (const void *pa, const void *pb) /* Add an operand entry to *OPS for the tree operand OP. */ static void -add_to_ops_vec (vec *ops, tree op) +add_to_ops_vec (vec *ops, tree op, int rank = 0) { operand_entry *oe = operand_entry_pool.allocate (); oe->op = op; - oe->rank = get_rank (op); + oe->rank = rank ? rank : get_rank (op); oe->id = next_operand_entry_id++; oe->count = 1; ops->safe_push (oe); @@ -1824,7 +1824,7 @@ transform_add_to_multiply (gimple *stmt, vec *ops) else insert_stmt_after (mul_stmt, def_stmt); gimple_set_visited (mul_stmt, true); - add_to_ops_vec (ops, tmp); + add_to_ops_vec (ops, tmp, bb_rank [gimple_bb (stmt)->index]); changed = true; }