From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30494 invoked by alias); 23 Apr 2014 15:31:04 -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 30482 invoked by uid 89); 23 Apr 2014 15:31:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 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; Wed, 23 Apr 2014 15:31:03 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s3NFUwQZ026019 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 23 Apr 2014 11:31:01 -0400 Received: from Mair.local (vpn-48-12.rdu2.redhat.com [10.10.48.12]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s3NF05p9020410; Wed, 23 Apr 2014 11:00:07 -0400 Message-ID: <5357D588.6000202@redhat.com> Date: Wed, 23 Apr 2014 15:33:00 -0000 From: Vladimir Makarov User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Robert Suchanek , "gcc-patches@gcc.gnu.org" , rdsandiford@googlemail.com Subject: Re: [RFC][PATCH][MIPS] Patch to enable LRA for MIPS backend References: <87d2h51dm6.fsf@talisman.default> <87d2gqfb7t.fsf@talisman.default> <87ob02jodp.fsf@talisman.default> <87fvl6hnw2.fsf@talisman.default> In-Reply-To: <87fvl6hnw2.fsf@talisman.default> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-04/txt/msg01441.txt.bz2 On 2014-04-21, 8:23 AM, Richard Sandiford wrote: > Robert Suchanek writes: >>>>> Did you see the failures even after your mips_regno_mode_ok_for_base_p >>>>> change? LRA should know how to reload a "W" address. >>>> >>>> Yes but I realize there is more. It fails because $sp is now included >>>> in BASE_REG_CLASS and "W" is based on it. However, I suppose that >>>> it would be too eager to say it is wrong and likely there is >>>> something missing >>>> in LRA if we want to keep all alternatives. Currently there is no check >>>> if a reloaded operand has a valid address, use of $sp in lbu/lhu cases. >>>> Even if we added extra checks we are less likely to benefit as we need >>>> to reload the base into register. >>> >>> Not sure what you mean, sorry. "W" exists specifically to exclude >>> $sp-based and $pc-based addresses. LRA AFAIK should already be able >>> to reload addresses that are valid in the TARGET_LEGITIMATE_ADDRESS_P >>> sense but which do not match the constraints for a particular insn. >>> >>> Can you remember one of the tests that fails? >> >> I couldn't trigger the problem with the original testcase but found >> another one that reveals it. The following needs to compiled with >> -mips32r2 -mips16 -Os: >> >> struct { int addr; } c; >> struct command { int args[1]; }; >> unsigned short a; >> >> fn1 (struct command *p1) >> { >> unsigned short d; >> d = fn2 (); >> a = p1->args[0]; >> fn3 (a); >> if (c.addr) >> { >> fn4 (p1->args[0]); >> return; >> } >> (&c)->addr = fn5 (); >> fn6 (d); >> } > > Thanks. > >> Not sure how the constraint would/should exclude $sp-based address in >> LRA. In this particular case, a spilled pseudo is changed to memory >> giving the following RTL form: >> >> (insn 30 29 31 4 (set (reg:SI 4 $4) >> (and:SI (mem/c:SI (plus:SI (reg/f:SI 78 $frame) >> (const_int 16 [0x10])) [7 %sfp+16 S4 A32]) >> (const_int 65535 [0xffff]))) shell.i:17 161 {*andsi3_mips16} >> (expr_list:REG_DEAD (reg:SI 194 [ D.1469 ]) >> (nil))) >> >> The operand 1 during alternative selection is not marked as a bad >> operand as it is a memory operand. $frame appears to be fine as it >> could be eliminated later to hard register. No reloads are inserted >> for the instructions concerned. Unless, $frame should be temporarily >> eliminated and then a reload would be inserted? > > Yeah, I think the lack of elimination is the problem. process_address > eliminates $frame temporarily before checking whether the address > is valid, but the places that check EXTRA_CONSTRAINT_STR pass the > original uneliminated address. So the legitimate_address_p hook sees > the $sp-based address but the "W" constraint only sees the $frame-based > address (which might or might not be valid, depending on whether $frame > is eliminated to the stack or hard frame pointer). I think the constraints > should see the eliminated address too. > > This patch seems to fix it for me. Tested on x86_64-linux-gnu. > Vlad, is this OK for trunk? > > BTW, we might want to define something like: > > #define MODE_BASE_REG_CLASS(MODE) \ > (TARGET_MIPS16 \ > ? ((MODE) == SImode || (MODE) == DImode ? M16_SP_REGS : M16_REGS) \ > : GR_REGS) > > instead of BASE_REG_CLASS. It might lead to slightly better code > (or not -- if it doesn't then don't bother :-)). > > If this patch is OK then I think the only thing blocking the switch > to LRA is the asm-subreg-1.c failure. I think it'd be fine to XFAIL > that test on MIPS for now, until there's a consensus about what "X" means > for asms. > > > gcc/ > * lra-constraints.c (valid_address_p): Move earlier in file. > Add a constraint argument to the address_info version. > (satisfies_memory_constraint_p): New function. > (satisfies_address_constraint_p): Likewise. > (process_alt_operands, curr_insn_transform): Use them. > (process_address): Pass the constraint to valid_address_p when > checking address operands. > > Yes, it looks ok for me, Richard. Thanks on working on this. I am on vacation till May 4th. If the patch results in problems on other targets, I hope you revert it. But to be honest, I believe it is very safe and don't expect any problems at all.