From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 108986 invoked by alias); 18 Sep 2019 09:41:49 -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 108978 invoked by uid 89); 18 Sep 2019 09:41:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-9.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 spammy=H*i:sk:CAFiYyc, H*f:_Qcw, H*i:pE3hP84tz, H*f:pE3hP84tz X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.110.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 18 Sep 2019 09:41:47 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 66FFB337; Wed, 18 Sep 2019 02:41:45 -0700 (PDT) Received: from localhost (unknown [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E5C243F59C; Wed, 18 Sep 2019 02:41:44 -0700 (PDT) From: Richard Sandiford To: Richard Biener Mail-Followup-To: Richard Biener ,GCC Patches , richard.sandiford@arm.com Cc: GCC Patches Subject: Re: Make assemble_real generate canonical CONST_INTs References: Date: Wed, 18 Sep 2019 09:41:00 -0000 In-Reply-To: (Richard Biener's message of "Wed, 18 Sep 2019 10:41:52 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg01091.txt.bz2 Richard Biener writes: > On Tue, Sep 17, 2019 at 4:33 PM Richard Sandiford > wrote: >> >> assemble_real used GEN_INT to create integers directly from the >> longs returned by real_to_target. assemble_integer then went on >> to interpret the const_ints as though they had the mode corresponding >> to the accompanying size parameter: >> >> imode = mode_for_size (size * BITS_PER_UNIT, mclass, 0).require (); >> >> for (i = 0; i < size; i += subsize) >> { >> rtx partial = simplify_subreg (omode, x, imode, i); >> >> But in the assemble_real case, X might not be canonical for IMODE. >> >> If the interface to assemble_integer is supposed to allow outputting >> (say) the low 4 bytes of a DImode integer, then the simplify_subreg >> above is wrong. But if the number of bytes passed to assemble_integer >> is supposed to be the number of bytes that the integer actually contains, >> assemble_real is wrong. >> >> This patch takes the latter interpretation and makes assemble_real >> generate const_ints that are canonical for the number of bytes passed. >> >> The flip_storage_order handling assumes that each long is a full >> SImode, which e.g. excludes BITS_PER_UNIT != 8 and float formats >> whose memory size is not a multiple of 32 bits (which includes >> HFmode at least). The patch therefore leaves that code alone. >> If interpreting each integer as SImode is correct, the const_ints >> that it generates are also correct. >> >> Tested on aarch64-linux-gnu and x86_64-linux-gnu. Also tested >> by making sure that there were no new errors from a range of >> cross-built targets. OK to install? >> >> Richard >> >> >> 2019-09-17 Richard Sandiford >> >> gcc/ >> * varasm.c (assemble_real): Generate canonical const_ints. >> >> Index: gcc/varasm.c >> =================================================================== >> --- gcc/varasm.c 2019-09-05 08:49:30.829739618 +0100 >> +++ gcc/varasm.c 2019-09-17 15:30:10.400740515 +0100 >> @@ -2873,25 +2873,27 @@ assemble_real (REAL_VALUE_TYPE d, scalar >> real_to_target (data, &d, mode); >> >> /* Put out the first word with the specified alignment. */ >> + unsigned int chunk_nunits = MIN (nunits, units_per); >> if (reverse) >> elt = flip_storage_order (SImode, gen_int_mode (data[nelts - 1], SImode)); >> else >> - elt = GEN_INT (data[0]); >> - assemble_integer (elt, MIN (nunits, units_per), align, 1); >> - nunits -= units_per; >> + elt = GEN_INT (sext_hwi (data[0], chunk_nunits * BITS_PER_UNIT)); > > why the appearant difference between the storage-order flipping > variant using gen_int_mode vs. the GEN_INT with sext_hwi? > Can't we use gen_int_mode in the non-flipping path and be done with that? Yeah, I mentioned this in the covering note. The flip_storage_order stuff only seems to work for floats that are a multiple of 32 bits in size, so it doesn't e.g. handle HFmode or 80-bit floats, whereas the new "else" does. Hard-coding SImode also hard-codes BITS_PER_UNIT==8, unlike the "else". So if anything, it's flip_storage_order that might need to change to avoid hard-coding SImode. That doesn't look like a trivial change though. E.g. the number of bytes passed to assemble_integer would need to match the number of bytes in data[nelts - 1] rather than data[0]. The alignment code below would also need to be adjusted. Fixing that (if it is a bug) seems like a separate change and TBH I'd rather not touch it here. Thanks, Richard > >> + assemble_integer (elt, chunk_nunits, align, 1); >> + nunits -= chunk_nunits; >> >> /* Subsequent words need only 32-bit alignment. */ >> align = min_align (align, 32); >> >> for (int i = 1; i < nelts; i++) >> { >> + chunk_nunits = MIN (nunits, units_per); >> if (reverse) >> elt = flip_storage_order (SImode, >> gen_int_mode (data[nelts - 1 - i], SImode)); >> else >> - elt = GEN_INT (data[i]); >> - assemble_integer (elt, MIN (nunits, units_per), align, 1); >> - nunits -= units_per; >> + elt = GEN_INT (sext_hwi (data[i], chunk_nunits * BITS_PER_UNIT)); >> + assemble_integer (elt, chunk_nunits, align, 1); >> + nunits -= chunk_nunits; >> } >> } >>