* Make assemble_real generate canonical CONST_INTs
@ 2019-09-17 14:33 Richard Sandiford
2019-09-18 8:42 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Richard Sandiford @ 2019-09-17 14:33 UTC (permalink / raw)
To: gcc-patches
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));
+ 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;
}
}
\f
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Make assemble_real generate canonical CONST_INTs
2019-09-17 14:33 Make assemble_real generate canonical CONST_INTs Richard Sandiford
@ 2019-09-18 8:42 ` Richard Biener
2019-09-18 9:41 ` Richard Sandiford
0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2019-09-18 8:42 UTC (permalink / raw)
To: Richard Sandiford; +Cc: GCC Patches
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?
> + 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;
> }
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Make assemble_real generate canonical CONST_INTs
2019-09-18 8:42 ` Richard Biener
@ 2019-09-18 9:41 ` Richard Sandiford
2019-09-18 10:07 ` Richard Biener
2019-09-20 13:41 ` Christophe Lyon
0 siblings, 2 replies; 6+ messages in thread
From: Richard Sandiford @ 2019-09-18 9:41 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
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.
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;
>> }
>> }
>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Make assemble_real generate canonical CONST_INTs
2019-09-18 9:41 ` Richard Sandiford
@ 2019-09-18 10:07 ` Richard Biener
2019-09-20 13:41 ` Christophe Lyon
1 sibling, 0 replies; 6+ messages in thread
From: Richard Biener @ 2019-09-18 10:07 UTC (permalink / raw)
To: Richard Sandiford; +Cc: GCC Patches
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;
> >> }
> >> }
> >>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Make assemble_real generate canonical CONST_INTs
2019-09-18 9:41 ` Richard Sandiford
2019-09-18 10:07 ` Richard Biener
@ 2019-09-20 13:41 ` Christophe Lyon
2019-09-26 10:48 ` Richard Sandiford
1 sibling, 1 reply; 6+ messages in thread
From: Christophe Lyon @ 2019-09-20 13:41 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Richard Biener, GCC Patches
On Wed, 18 Sep 2019 at 11:41, 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.
>
Hi Richard,
I suspect you've probably noticed already, but in case you haven't:
this patch causes a regression on arm:
FAIL: gcc.target/arm/fp16-compile-alt-3.c scan-assembler \t.short\t49152
FAIL: gcc.target/arm/fp16-compile-ieee-3.c scan-assembler \t.short\t49152
Christophe
> 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;
> >> }
> >> }
> >>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Make assemble_real generate canonical CONST_INTs
2019-09-20 13:41 ` Christophe Lyon
@ 2019-09-26 10:48 ` Richard Sandiford
0 siblings, 0 replies; 6+ messages in thread
From: Richard Sandiford @ 2019-09-26 10:48 UTC (permalink / raw)
To: Christophe Lyon; +Cc: Richard Biener, GCC Patches
Christophe Lyon <christophe.lyon@linaro.org> writes:
> On Wed, 18 Sep 2019 at 11:41, 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.
>>
>
> Hi Richard,
>
> I suspect you've probably noticed already, but in case you haven't:
> this patch causes a regression on arm:
> FAIL: gcc.target/arm/fp16-compile-alt-3.c scan-assembler \t.short\t49152
> FAIL: gcc.target/arm/fp16-compile-ieee-3.c scan-assembler \t.short\t49152
Hadn't noticed that actually (but should have) -- thanks for the heads up.
I've applied the below as obvious after testing on armeb-eabi.
Richard
2019-09-26 Richard Sandiford <richard.sandiford@arm.com>
gcc/testsuite/
* gcc.target/arm/fp16-compile-alt-3.c: Expect (__fp16) -2.0
to be written as a negative short rather than a positive one.
* gcc.target/arm/fp16-compile-ieee-3.c: Likewise.
Index: gcc/testsuite/gcc.target/arm/fp16-compile-alt-3.c
===================================================================
--- gcc/testsuite/gcc.target/arm/fp16-compile-alt-3.c 2019-03-08 18:14:28.836998325 +0000
+++ gcc/testsuite/gcc.target/arm/fp16-compile-alt-3.c 2019-09-26 11:42:47.502378676 +0100
@@ -7,4 +7,4 @@
__fp16 xx = -2.0;
/* { dg-final { scan-assembler "\t.size\txx, 2" } } */
-/* { dg-final { scan-assembler "\t.short\t49152" } } */
+/* { dg-final { scan-assembler "\t.short\t-16384" } } */
Index: gcc/testsuite/gcc.target/arm/fp16-compile-ieee-3.c
===================================================================
--- gcc/testsuite/gcc.target/arm/fp16-compile-ieee-3.c 2019-03-08 18:14:28.732998720 +0000
+++ gcc/testsuite/gcc.target/arm/fp16-compile-ieee-3.c 2019-09-26 11:42:47.506378645 +0100
@@ -6,4 +6,4 @@
__fp16 xx = -2.0;
/* { dg-final { scan-assembler "\t.size\txx, 2" } } */
-/* { dg-final { scan-assembler "\t.short\t49152" } } */
+/* { dg-final { scan-assembler "\t.short\t-16384" } } */
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-26 10:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-17 14:33 Make assemble_real generate canonical CONST_INTs Richard Sandiford
2019-09-18 8:42 ` Richard Biener
2019-09-18 9:41 ` Richard Sandiford
2019-09-18 10:07 ` Richard Biener
2019-09-20 13:41 ` Christophe Lyon
2019-09-26 10:48 ` Richard Sandiford
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).