From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 46557 invoked by alias); 18 Sep 2019 10:07:46 -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 46549 invoked by uid 89); 18 Sep 2019 10:07:46 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-7.4 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=H*f:sk:mptblvi, H*i:sk:mptblvi X-HELO: mail-lj1-f175.google.com Received: from mail-lj1-f175.google.com (HELO mail-lj1-f175.google.com) (209.85.208.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 18 Sep 2019 10:07:44 +0000 Received: by mail-lj1-f175.google.com with SMTP id l21so4874048lje.4 for ; Wed, 18 Sep 2019 03:07:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=5TYAMr8AlTYaJISu8TbSkBlQogc8aOIcsrUMNlxv6WM=; b=Yras9k80/K2oO10CVYaCbHOSJwv/C7KyiAdT4IEDEmNfF4/H/wNyFXwpDa8qYXM2Rs sy5pymTWTerXZeSoSfEM+daIaJiGo26YDILuatdeVfaZWF5fBkNssyBDv2w1jShSIFUA mMbDwKSVhzRkeHenYdrKm8KyHChdCkvdteVOklKpAWeAWgu7PpQlC7BqnaPc2KiYTZEy iPxu92DQ7wtNECRMd8tWiVgcVtW4trbOoavK4wc3gTcy+2xNqmCPAvZbdiSX+z1tF2iK qMLBQbA+KQLxmjFFHbj45K21XI+Cqjylg60IojJaeHRQm+BWyK3n9hyRuqFL14ioQlQx 1STA== MIME-Version: 1.0 References: In-Reply-To: From: Richard Biener Date: Wed, 18 Sep 2019 10:07:00 -0000 Message-ID: Subject: Re: Make assemble_real generate canonical CONST_INTs To: Richard Sandiford Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg01093.txt.bz2 On Wed, Sep 18, 2019 at 11:41 AM Richard Sandiford wrote: > > 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. Hmm, ok. Patch is OK then. Thanks, Richard. > 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; > >> } > >> } > >>