From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20768 invoked by alias); 23 Jun 2014 09:41:16 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 20741 invoked by uid 89); 23 Jun 2014 09:41:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wi0-f182.google.com Received: from mail-wi0-f182.google.com (HELO mail-wi0-f182.google.com) (209.85.212.182) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 23 Jun 2014 09:40:43 +0000 Received: by mail-wi0-f182.google.com with SMTP id bs8so3825385wib.15 for ; Mon, 23 Jun 2014 02:40:40 -0700 (PDT) X-Received: by 10.194.119.228 with SMTP id kx4mr2491012wjb.108.1403516439587; Mon, 23 Jun 2014 02:40:39 -0700 (PDT) Received: from localhost ([2.26.169.52]) by mx.google.com with ESMTPSA id na4sm31725627wic.21.2014.06.23.02.40.38 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 23 Jun 2014 02:40:38 -0700 (PDT) From: Richard Sandiford To: Sameera Deshpande Mail-Followup-To: Sameera Deshpande ,Matthew Fortune , "gcc-patches\@gcc.gnu.org" , rdsandiford@googlemail.com Cc: Matthew Fortune , "gcc-patches\@gcc.gnu.org" Subject: Re: [PATCH][MIPS] Enable load-load/store-store bonding References: <38C8F1E431EDD94A82971C543A11B4FEE0A588@PUMAIL01.pu.imgtec.org> <8761ju8c2k.fsf@talisman.default> <38C8F1E431EDD94A82971C543A11B4FEE0BB2F@PUMAIL01.pu.imgtec.org> Date: Mon, 23 Jun 2014 09:41:00 -0000 In-Reply-To: <38C8F1E431EDD94A82971C543A11B4FEE0BB2F@PUMAIL01.pu.imgtec.org> (Sameera Deshpande's message of "Mon, 23 Jun 2014 09:17:55 +0000") Message-ID: <8761js6k15.fsf@talisman.default> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2014-06/txt/msg01742.txt.bz2 Sameera Deshpande writes: >> > + if (TARGET_FIX_24K && TUNE_P5600) >> > + error ("unsupported combination: %s", "-mtune=p5600 -mfix-24k"); >> > + >> > /* Save the base compression state and process flags as though we >> > were generating uncompressed code. */ >> > mips_base_compression_flags = TARGET_COMPRESSION; >> >> Although it's a bit of an odd combination, we need to accept -mfix-24k - >> mtune=p5600 and continue to implement the 24k workarounds. >> The idea is that a distributor can build for a common base architecture, add - >> mfix- options for processors that might run the code, and add -mtune= for >> the processor that's most of interest optimisation-wise. >> >> We should just make the pairing of stores conditional on !TARGET_FIX_24K. > We had offline discussion based on your comment. There is additional > view on the same. > Only ISAs mips32r2, mips32r3 and mips32r5 support P5600. Remaining ISAs > do not support P5600. > For mips32r2 (24K) and mips32r3 (micromips), load-store pairing is > implemented separately, and hence, as you suggested, P5600 Ld-ST bonding > optimization should not be enabled for them. > So, is it fine if I emit error for any ISAs other than mips32r2, > mips32r3 and mips32r5 when P5600 is enabled, or the compilation should > continue by emitting warning and disabling P5600? No, the point is that we have two separate concepts: ISA and optimisation target. -mipsN and -march=N control the ISA (which instructions are available) and -mtune=M controls optimisation decisions within the constraints of that N, such as scheduling and the cost of things like multiplication and division. E.g. you could have -mips2 -mtune=p5600 -mfix-24k: generate MIPS II-compatible code, optimise it for p5600, but make sure that 24k workarounds are used. The code would run correctly on any MIPS II-compatible processor without known errata and also on the 24k. >> > + >> > +#define ENABLE_LD_ST_PAIRING \ >> > + (TARGET_ENABLE_LD_ST_PAIRING && TUNE_P5600) >> >> The patch requires -mld-st-pairing to be passed explicitly even for - >> mtune=p5600. Is that because it's not a consistent enough win for us to >> enable it by default? It sounded from the description like it should be an >> improvement more often that not. >> >> We should allow pairing even without -mtune=p5600. > Performance testing for this patch is not yet done. > If the patch proves beneficial in most of the testcases (which we > believe will do on P5600) we will enable this optimization by default > for P5600 - in which case this option can be removed. OK. Sending the patch for comments before performance testing is fine, but I think it'd be better to commit the patch only after the testing is done, since otherwise the patch might need to be tweaked. I don't see any problem with keeping the option in case people want to experiment with it. I just think the patch should only go in once it can be enabled by default for p5600. I.e. the option would exist to turn off the pairing. Not having the option is fine too of course. >> Are QImodes not paired in the same way? If so, it'd be worth adding a >> comment above the define_mode_iterator saying that QI is deliberately >> excluded. > The P5600 datasheet mentions bonding of load/stores in HI, SI, SF and DF > modes only. Hence QI mode is excluded. I will add the comment on the > iterator. Thanks. Richard