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: 624010 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 3r9VVb4xjCz9t3V for ; Thu, 19 May 2016 22:19:18 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=FS8QjM4T; 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 :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=MNhSsJybMZFJxCuYyX z3BE5otVRAq8oAcFkXF0z9Q0yHMFfdb97lQmBdpm1O3SZcOxH6UMIaLSbcTBTTJy I2wr9vcxC4tYtipN7KvR6ZaJl6JHFt9VlW6H16XSN44qSgNLd44rWP+XhY2Br6nS JEVaH8rvMcSBj5056zQ9R+MUI= 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=FS8QjM4ToFIOw4HKzkUr13tBTq4Gh8SlHbfBKzu+5thpdDNSEJKuG8yp DgBIFYWxLVBrGoRJIuLwNyqH0tFl+NqzdP/Z1oSEUBdTaS0JeuA4PH41Be8PszZk GAqcA0bMbVhI83Yt6UKPO5AGW5L8PHekEv+uWN337baHftzfndc= 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; }