Make assemble_real generate canonical CONST_INTs

[Richard Biener]

On Wed, Sep 18, 2019 at 11:41 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Richard Biener <richard.guenther@gmail.com> writes:
> > On Tue, Sep 17, 2019 at 4:33 PM Richard Sandiford
> > <richard.sandiford@arm.com> 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  <richard.sandiford@arm.com>
> >>
> >> 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;
> >>      }
> >>  }
> >>