From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19239 invoked by alias); 30 Apr 2012 14:57:45 -0000 Received: (qmail 19227 invoked by uid 22791); 30 Apr 2012 14:57:44 -0000 X-SWARE-Spam-Status: No, hits=-1.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from service87.mimecast.com (HELO service87.mimecast.com) (91.220.42.44) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 30 Apr 2012 14:57:21 +0000 Received: from cam-owa2.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.21]) by service87.mimecast.com; Mon, 30 Apr 2012 15:57:18 +0100 Received: from [10.1.69.67] ([10.1.255.212]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Mon, 30 Apr 2012 15:58:42 +0100 Message-ID: <4F9EA849.8020807@arm.com> Date: Mon, 30 Apr 2012 14:57:00 -0000 From: Richard Earnshaw User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:11.0) Gecko/20120327 Thunderbird/11.0.1 MIME-Version: 1.0 To: Richard Earnshaw , Jim MacArthur , "gcc-patches@gcc.gnu.org" , rdsandiford@googlemail.com Subject: Re: [patch] More thorough checking in reg_fits_class_p References: <4F994B99.1010606@arm.com> <4F9E9772.8080700@arm.com> <4F9E9DCB.7010206@arm.com> In-Reply-To: X-MC-Unique: 112043015571805101 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes 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 X-SW-Source: 2012-04/txt/msg01883.txt.bz2 On 30/04/12 15:39, Richard Sandiford wrote: > Richard Earnshaw writes: >> On 30/04/12 15:07, Richard Sandiford wrote: >>> Richard Earnshaw writes: >>>> On 26/04/12 14:20, Jim MacArthur wrote: >>>>> The current code in reg_fits_class_p appears to be incorrect; since=20 >>>>> offset may be negative, it's necessary to check both ends of the rang= e=20 >>>>> otherwise an array overrun or underrun may occur when calling=20 >>>>> in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each= =20 >>>>> register in the range of regno .. regno+offset. >>>>> >>>>> A negative offset can occur on a big-endian target. For example, on=20 >>>>> AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1. >>>>> >>>>> We discovered this problem while developing unrelated code for=20 >>>>> big-endian support in the AArch64 back end. >>>>> >>>>> I've tested this with an x86 bootstrap which shows no errors, and wit= h=20 >>>>> our own AArch64 back end. >>>>> >>>>> Ok for trunk? >>>>> >>>>> gcc/Changelog entry: >>>>> >>>>> 2012-04-26 Jim MacArthur >>>>> * recog.c (reg_fits_class_p): Check every register between regn= o and >>>>> regno+offset is in the hard register set. >>>>> >>>> >>>> OK. >>>> >>>> R. >>>> >>>>> >>>>> reg-fits-class-9 >>>>> >>>>> >>>>> diff --git a/gcc/recog.c b/gcc/recog.c >>>>> index 8fb96a0..825bfb1 100644 >>>>> --- a/gcc/recog.c >>>>> +++ b/gcc/recog.c >>>>> @@ -2759,14 +2759,28 @@ bool >>>>> reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset, >>>>> enum machine_mode mode) >>>>> { >>>>> - int regno =3D REGNO (operand); >>>>> + unsigned int regno =3D REGNO (operand); >>>>>=20=20 >>>>> if (cl =3D=3D NO_REGS) >>>>> return false; >>>>>=20=20 >>>>> - return (HARD_REGISTER_NUM_P (regno) >>>>> - && in_hard_reg_set_p (reg_class_contents[(int) cl], >>>>> - mode, regno + offset)); >>>>> + /* We should not assume all registers in the range regno to regno = + offset are >>>>> + valid just because each end of the range is. */ >>>>> + if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + of= fset)) >>>>> + { >>>>> + unsigned int i; >>>>> + >>>>> + unsigned int start =3D MIN (regno, regno + offset); >>>>> + unsigned int end =3D MAX (regno, regno + offset); >>>>> + for (i =3D start; i <=3D end; i++) >>>>> + { >>>>> + if (!in_hard_reg_set_p (reg_class_contents[(int) cl], >>>>> + mode, i)) >>>>> + return false; >>>>> + } >>> >>> This doesn't look right to me. We should still only need to check >>> in_hard_reg_set_p for one register number. I'd have expected >>> something like: >>> >>> return (HARD_REGISTER_NUM_P (regno) >>> && HARD_REGISTER_NUM_P (regno + offset) >>> && in_hard_reg_set_p (reg_class_contents[(int) cl], >>> mode, regno + offset)); >>> >>> instead. >>> >>> Richard >>> >> >> There's no guarantee that all registers in a set are contiguous; ARM for >> example doesn't make that guarantee, since SP is not a GP register, but >> R12 and R14 are. >=20 > Sorry, I don't follow. My point was that in_hard_reg_set_p (C, M, R1) > tests whether every register required to store a value of mode M starting > at R1 fits in C. Which is what we want to know. >=20 > Whether the intermediate registers (between regno and regno + offset) > are even valid for MODE shouldn't matter. I don't think it makes > conceptual sense to call: >=20 > if (!in_hard_reg_set_p (reg_class_contents[(int) cl], > mode, i)) >=20 > for regno < i < regno + offset (or regno + offset < i < regno), > because we're not trying to construct a value of mode MODE > in that register. >=20 > Richard >=20 You're right, of course. I'd missed that in my initial review; and hence my follow-up suggestion. It's not particularly interesting whether or not HARD_REGISTER_NUM_P (REGNO (operand)) is true, only whether REGNO(operand) + offset ... REGNO(operand) + offset + NUM_REGS(mode) -1 is. R.