From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id DEF093861841 for ; Fri, 6 Nov 2020 21:29:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DEF093861841 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-3-ZAU377P-PbSi7Jzo1F7KNg-1; Fri, 06 Nov 2020 16:29:46 -0500 X-MC-Unique: ZAU377P-PbSi7Jzo1F7KNg-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 9BA8786ABCF for ; Fri, 6 Nov 2020 21:29:45 +0000 (UTC) Received: from localhost.localdomain (ovpn-114-181.phx2.redhat.com [10.3.114.181]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6D7131002C1C for ; Fri, 6 Nov 2020 21:29:45 +0000 (UTC) Subject: Re: ping x2 [PATCH 0/5] MSP430: Implement macros to describe relative costs of operations To: gcc-patches@gcc.gnu.org References: <20200723154356.63ws2xairlmdufji@jozef-acer-manjaro> <20200807110259.fgo5y6eb6a3pwvuk@jozef-acer-manjaro> <20200915203022.6lf3pkkaj5ojybmi@jozef-acer-manjaro> <9ae3290e-c76f-f756-fba6-74c59952da14@redhat.com> <20201106212341.orcdltocuc5mmhnv@jozef-acer-manjaro> From: Jeff Law Message-ID: Date: Fri, 6 Nov 2020 14:29:44 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1 MIME-Version: 1.0 In-Reply-To: <20201106212341.orcdltocuc5mmhnv@jozef-acer-manjaro> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-US X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Nov 2020 21:29:50 -0000 On 11/6/20 2:23 PM, Jozef Lawrynowicz wrote: > On Fri, Nov 06, 2020 at 01:53:19PM -0700, Jeff Law via Gcc-patches wrote: >> On 9/15/20 2:30 PM, Jozef Lawrynowicz wrote: >>> Ping x2 for below. >>> >>> On Fri, Aug 07, 2020 at 12:02:59PM +0100, Jozef Lawrynowicz wrote: >>>> Pinging for this series of patches. >>>> Attached all patches to this mail with the ammended patch 4 thanks to >>>> Segher's review. >>>> >>>> Thanks, >>>> Jozef >>>> >>>> On Thu, Jul 23, 2020 at 04:43:56PM +0100, Jozef Lawrynowicz wrote: >>>>> The following series of patches for MSP430 implement some of the target >>>>> macros used to determine the relative costs of operations. >>>>> >>>>> To give an indication of the overall effect of these changes on >>>>> codesize, below are some size statistics collected from all the >>>>> executable files from execute.exp that are built at -Os. >>>>> There are around 1470 such tests (depending on the configuration). >>>>> >>>>> The percentage change (((new - old)/old) * 100) in text size is calculated >>>>> for each test and the given metric is applied to that overall set of data. >>>>> >>>>> Configuration | Mean (%) | Median (%) | Delta < 0 (count) | Delta > 0 (count) >>>>> ----------------------------------------------------------------------------- >>>>> -mcpu=msp430 | -2.4 | -2.7 | 1454 | 17 >>>>> -mcpu=msp430x | -2.3 | -2.4 | 1460 | 10 >>>>> -mlarge | -1.7 | -1.9 | 1412 | 37 >>>>> >>>>> Successfully regtested on trunk for msp430-elf, ok to apply? >>>>> >>>>> Jozef Lawrynowicz (5): >>>>> MSP430: Implement TARGET_MEMORY_MOVE_COST >>>>> MSP430: Implement TARGET_RTX_COSTS >>>>> MSP430: Add defaulting to the insn length attribute >>>>> MSP430: Implement TARGET_INSN_COST >>>>> MSP430: Skip index-1.c test >>>>> >>>>> gcc/config/msp430/msp430-protos.h | 5 +- >>>>> gcc/config/msp430/msp430.c | 867 ++++++++++++++++-- >>>>> gcc/config/msp430/msp430.h | 13 + >>>>> gcc/config/msp430/msp430.md | 439 +++++++-- >>>>> gcc/config/msp430/msp430.opt | 4 + >>>>> gcc/config/msp430/predicates.md | 13 + >>>>> gcc/testsuite/gcc.c-torture/execute/index-1.c | 2 + >>>>> 7 files changed, 1206 insertions(+), 137 deletions(-) >> [ ... ] >> >> So it's a series of 5 patches.  They LGTM.    And if  there's minor >> updates needed to address issues found once they're on the trunk, the >> consider those updates pre-approved. > Spooky, I pinged these patches the minute before you replied to this one. Yea, I saw your ping when I went to my inbox after reviewing the change ;-) > >> Note that defining LOGICAL_OP_NON_SHORT_CIRCUIT and BRANCH_COST impact >> gimple code generation.  I'm a bit surprised there wasn't more fallout >> in the existing tests. > IIRC, when I tried messing with LOGICAL_OP_NON_SHORT_CIRCUIT and > BRANCH_COST before the full costs were implemented, they never had any > effect. I'm going to hand wave here and say that whatever behavior these > macros were affecting before the costs were implemented, was actually > behaving as it should. Then with the full costs implemented, I had to > tweak them to get back to that optimal state. > > Also, the only conditional instructions available for MSP430 are > cbranch, i.e. there is never a choice between a cbranch and a cstore. > I was surprised BRANCH_COST had any effect, but after looking at output > assembly, it appeared that it did affect the number of cbranch insns > emitted, vs just other insns to move data about. It tends to twiddle some of the jump threading tests which are sensitive to the jumping structure (surprise).  We try to handle both cases, but it's proven pretty fragile thorugh the years. > > The changes did fix these optimization tests: > gcc.dg/tree-ssa/reassoc-33.c scan-tree-dump-times reassoc1 "Optimizing range tests" 3 > gcc.dg/tree-ssa/reassoc-34.c scan-tree-dump-times reassoc1 "Optimizing range tests" 1 > gcc.dg/tree-ssa/reassoc-35.c scan-tree-dump-times reassoc1 "Optimizing range tests" 1 Yea, those would be well within the realm of expected for changing those macros since they'd change the decisons  for expanding a range test either as a series of conditionals or a series of logical ops and a single conditional.  THat in turn heavily influences optimize_range_test in tree-ssa-reassoc > gcc.dg/tree-ssa/reassoc-36.c scan-tree-dump-times reassoc1 "Optimizing range tests" 1 > > Thanks for the review, I'll watch for fallout after installing on trunk. Likewise.  I think msp430 was running in my tester while I was reviewing the changes, so tomorrow's run should be interesting to review. jeff