[GCC/ARM/gcc-7-branch] Backport PR71607

This patch tackles the issue reported in PR71607. This patch takes a different
approach for disabling the creation of literal pools. Instead of disabling the
patterns that would normally transform the rtl into actual literal pools, it
disables the creation of this literal pool rtl by making the target hook
TARGET_CANNOT_FORCE_CONST_MEM return true if arm_disable_literal_pool is true.
I added patterns to split floating point constants for both SF and DFmode. A
pattern to handle the addressing of label_refs had to be included as well since
all "memory_operand" patterns are disabled when TARGET_CANNOT_FORCE_CONST_MEM
returns true. Also the pattern for splitting 32-bit immediates had to be
changed, it was not accepting unsigned 32-bit unsigned integers with the MSB
set. I believe const_int_operand expects the mode of the operand to be set to
VOIDmode and not SImode. I have only changed it in the patterns that were
affecting this code, though I suggest looking into changing it in the rest of
the ARM backend.

Additionally, the use of thread-local storage is disabled if literal pools are
disabled, as there are no relocations for TLS variables and incorrect code is
generated as a result. The patch now emits a diagnostic in TLS-enabled
toolchains if a TLS symbol is found when -mpure-code or -mslow-flash-data are
enabled.

2017-05-25  Prakhar Bahuguna  <prakhar.bahuguna@arm.com>

	Backport from mainline
	2017-05-05  Andre Vieira  <andre.simoesdiasvieira@arm.com>
		    Prakhar Bahuguna  <prakhar.bahuguna@arm.com>

	gcc/
	PR target/71607
	* config/arm/arm.md (use_literal_pool): Remove.
	(64-bit immediate split): No longer takes cost into consideration
	if arm_disable_literal_pool is enabled.
	* config/arm/arm.c (arm_tls_referenced_p): Add diagnostic if TLS is
	used when arm_disable_literal_pool is enabled.
	(arm_max_const_double_inline_cost): Remove use of
	arm_disable_literal_pool.
	(push_minipool_fix): Add assert.
	(arm_reorg): Add return if arm_disable_literal_pool is enabled.
	* config/arm/vfp.md (no_literal_pool_df_immediate): New.
	(no_literal_pool_sf_immediate): New.

	2017-05-05  Andre Vieira  <andre.simoesdiasvieira@arm.com>
		    Thomas Preud'homme  <thomas.preudhomme@arm.com>
		    Prakhar Bahuguna  <prakhar.bahuguna@arm.com>

	gcc/testsuite/
	PR target/71607
	* gcc.target/arm/thumb2-slow-flash-data.c: Renamed to ...
	* gcc.target/arm/thumb2-slow-flash-data-1.c: ... this.
	* gcc.target/arm/thumb2-slow-flash-data-2.c: New.
	* gcc.target/arm/thumb2-slow-flash-data-3.c: New.
	* gcc.target/arm/thumb2-slow-flash-data-4.c: New.
	* gcc.target/arm/thumb2-slow-flash-data-5.c: New.
	* gcc.target/arm/tls-disable-literal-pool.c: New.

Testing done: Ran regression tests for arm-eabi-none for ARMv7-M and ARMv8-M
targets. A bootstrap for arm-none-linux-gnueabihf was also successful.

Prakhar Bahuguna <prakhar.bahuguna@arm.com> writes:
> This patch tackles the issue reported in PR71607. This patch takes a different
> approach for disabling the creation of literal pools. Instead of disabling the
> patterns that would normally transform the rtl into actual literal pools, it
> disables the creation of this literal pool rtl by making the target hook
> TARGET_CANNOT_FORCE_CONST_MEM return true if arm_disable_literal_pool is true.
> I added patterns to split floating point constants for both SF and DFmode. A
> pattern to handle the addressing of label_refs had to be included as well since
> all "memory_operand" patterns are disabled when TARGET_CANNOT_FORCE_CONST_MEM
> returns true. Also the pattern for splitting 32-bit immediates had to be
> changed, it was not accepting unsigned 32-bit unsigned integers with the MSB
> set. I believe const_int_operand expects the mode of the operand to be set to
> VOIDmode and not SImode. I have only changed it in the patterns that were
> affecting this code, though I suggest looking into changing it in the rest of
> the ARM backend.

I couldn't see the const_int_operand bit in the attached patch, but:
const_int_operand *should* usually be used with the logical integer
mode, such as SImode or DImode.

const_ints are supposed to be stored in sign-extended form, so a 32-bit
integer with the MSB set should be 0xffffffff80000000|x instead of
0x80000000|x.  It's a bug if you have one where that isn't true.

In the patch it looks like this could come from:

> diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md
> index befdea9edd9..d8f77e2ffe4 100644
> --- a/gcc/config/arm/vfp.md
> +++ b/gcc/config/arm/vfp.md
> @@ -2079,3 +2079,40 @@
>  ;; fmdhr et al (VFPv1)
>  ;; Support for xD (single precision only) variants.
>  ;; fmrrs, fmsrr
> +
> +;; Split an immediate DF move to two immediate SI moves.
> +(define_insn_and_split "no_literal_pool_df_immediate"
> +  [(set (match_operand:DF 0 "s_register_operand" "")
> +	(match_operand:DF 1 "const_double_operand" ""))]
> +  "TARGET_THUMB2 && arm_disable_literal_pool
> +  && !(TARGET_HARD_FLOAT && TARGET_VFP_DOUBLE
> +       && vfp3_const_double_rtx (operands[1]))"
> +  "#"
> +  "&& !reload_completed"
> +  [(set (subreg:SI (match_dup 1) 0) (match_dup 2))
> +   (set (subreg:SI (match_dup 1) 4) (match_dup 3))
> +   (set (match_dup 0) (match_dup 1))]
> +  "
> +  long buf[2];
> +  real_to_target (buf, CONST_DOUBLE_REAL_VALUE (operands[1]), DFmode);
> +  operands[2] = GEN_INT ((int) buf[0]);
> +  operands[3] = GEN_INT ((int) buf[1]);
> +  operands[1] = gen_reg_rtx (DFmode);
> +  ")
> +
> +;; Split an immediate SF move to one immediate SI move.
> +(define_insn_and_split "no_literal_pool_sf_immediate"
> +  [(set (match_operand:SF 0 "s_register_operand" "")
> +	(match_operand:SF 1 "const_double_operand" ""))]
> +  "TARGET_THUMB2 && arm_disable_literal_pool
> +  && !(TARGET_HARD_FLOAT && vfp3_const_double_rtx (operands[1]))"
> +  "#"
> +  "&& !reload_completed"
> +  [(set (subreg:SI (match_dup 1) 0) (match_dup 2))
> +   (set (match_dup 0) (match_dup 1))]
> +  "
> +  long buf;
> +  real_to_target (&buf, CONST_DOUBLE_REAL_VALUE (operands[1]), SFmode);
> +  operands[2] = GEN_INT ((int) buf);
> +  operands[1] = gen_reg_rtx (SFmode);
> +  ")

...these two splits, where the GEN_INTs should probably be:

  gen_int_mode (..., SImode);

instead.

Thanks,
Richard

On 31/05/2017 09:19:40, Richard Sandiford wrote:
> const_ints are supposed to be stored in sign-extended form, so a 32-bit
> integer with the MSB set should be 0xffffffff80000000|x instead of
> 0x80000000|x.  It's a bug if you have one where that isn't true.
> 
> In the patch it looks like this could come from:
> ...these two splits, where the GEN_INTs should probably be:
> 
>   gen_int_mode (..., SImode);
> 
> instead.

Hi Richard, thanks for the tip. Is there a test case that could produce an
incorrect result? I've attempted to create one using negative doubles and
floats but haven't succeeded.

Thanks,

Prakhar Bahuguna <prakhar.bahuguna@arm.com> writes:
> On 31/05/2017 09:19:40, Richard Sandiford wrote:
>> const_ints are supposed to be stored in sign-extended form, so a 32-bit
>> integer with the MSB set should be 0xffffffff80000000|x instead of
>> 0x80000000|x.  It's a bug if you have one where that isn't true.
>> 
>> In the patch it looks like this could come from:
>> ...these two splits, where the GEN_INTs should probably be:
>> 
>>   gen_int_mode (..., SImode);
>> 
>> instead.
>
> Hi Richard, thanks for the tip. Is there a test case that could produce an
> incorrect result? I've attempted to create one using negative doubles and
> floats but haven't succeeded.

Just to check, are you testing with --enable-checking=yes,rtl?

When the values you tried were split, did you get the sign-extended form
or the zero-extended form?

Thanks,
Richard

On 31/05/2017 14:11:43, Richard Sandiford wrote:
> Prakhar Bahuguna <prakhar.bahuguna@arm.com> writes:
> > On 31/05/2017 09:19:40, Richard Sandiford wrote:
> >> const_ints are supposed to be stored in sign-extended form, so a 32-bit
> >> integer with the MSB set should be 0xffffffff80000000|x instead of
> >> 0x80000000|x.  It's a bug if you have one where that isn't true.
> >> 
> >> In the patch it looks like this could come from:
> >> ...these two splits, where the GEN_INTs should probably be:
> >> 
> >>   gen_int_mode (..., SImode);
> >> 
> >> instead.
> >
> > Hi Richard, thanks for the tip. Is there a test case that could produce an
> > incorrect result? I've attempted to create one using negative doubles and
> > floats but haven't succeeded.
> 
> Just to check, are you testing with --enable-checking=yes,rtl?
> 
> When the values you tried were split, did you get the sign-extended form
> or the zero-extended form?
> 
> Thanks,
> Richard

I've now rebuilt with --enable-checking=yes,rtl and it appears that the split
values are being correctly sign-extended in the rtl and appear correctly in the
assembly.

However, if you believe it is safer to use gen_int_mode(), I'll respin the
patch accordingly.

Prakhar Bahuguna <prakhar.bahuguna@arm.com> writes:
> On 31/05/2017 14:11:43, Richard Sandiford wrote:
>> Prakhar Bahuguna <prakhar.bahuguna@arm.com> writes:
>> > On 31/05/2017 09:19:40, Richard Sandiford wrote:
>> >> const_ints are supposed to be stored in sign-extended form, so a 32-bit
>> >> integer with the MSB set should be 0xffffffff80000000|x instead of
>> >> 0x80000000|x.  It's a bug if you have one where that isn't true.
>> >> 
>> >> In the patch it looks like this could come from:
>> >> ...these two splits, where the GEN_INTs should probably be:
>> >> 
>> >>   gen_int_mode (..., SImode);
>> >> 
>> >> instead.
>> >
>> > Hi Richard, thanks for the tip. Is there a test case that could produce an
>> > incorrect result? I've attempted to create one using negative doubles and
>> > floats but haven't succeeded.
>> 
>> Just to check, are you testing with --enable-checking=yes,rtl?
>> 
>> When the values you tried were split, did you get the sign-extended form
>> or the zero-extended form?
>> 
>> Thanks,
>> Richard
>
> I've now rebuilt with --enable-checking=yes,rtl and it appears that the split
> values are being correctly sign-extended in the rtl and appear correctly in the
> assembly.
>
> However, if you believe it is safer to use gen_int_mode(), I'll respin the
> patch accordingly.

Yeah, I think it would be safer.  But if they were already correctly
sign-extended, then what did you mean by:

  Also the pattern for splitting 32-bit immediates had to be changed, it
  was not accepting unsigned 32-bit unsigned integers with the MSB
  set. I believe const_int_operand expects the mode of the operand to be
  set to VOIDmode and not SImode. I have only changed it in the patterns
  that were affecting this code, though I suggest looking into changing
  it in the rest of the ARM backend.

Thanks,
Richard

On 01/06/2017 07:15:47, Richard Sandiford wrote:
> Prakhar Bahuguna <prakhar.bahuguna@arm.com> writes:
> > On 31/05/2017 14:11:43, Richard Sandiford wrote:
> >> Prakhar Bahuguna <prakhar.bahuguna@arm.com> writes:
> >> > On 31/05/2017 09:19:40, Richard Sandiford wrote:
> >> >> const_ints are supposed to be stored in sign-extended form, so a 32-bit
> >> >> integer with the MSB set should be 0xffffffff80000000|x instead of
> >> >> 0x80000000|x.  It's a bug if you have one where that isn't true.
> >> >> 
> >> >> In the patch it looks like this could come from:
> >> >> ...these two splits, where the GEN_INTs should probably be:
> >> >> 
> >> >>   gen_int_mode (..., SImode);
> >> >> 
> >> >> instead.
> >> >
> >> > Hi Richard, thanks for the tip. Is there a test case that could produce an
> >> > incorrect result? I've attempted to create one using negative doubles and
> >> > floats but haven't succeeded.
> >> 
> >> Just to check, are you testing with --enable-checking=yes,rtl?
> >> 
> >> When the values you tried were split, did you get the sign-extended form
> >> or the zero-extended form?
> >> 
> >> Thanks,
> >> Richard
> >
> > I've now rebuilt with --enable-checking=yes,rtl and it appears that the split
> > values are being correctly sign-extended in the rtl and appear correctly in the
> > assembly.
> >
> > However, if you believe it is safer to use gen_int_mode(), I'll respin the
> > patch accordingly.
> 
> Yeah, I think it would be safer.  But if they were already correctly
> sign-extended, then what did you mean by:
> 
>   Also the pattern for splitting 32-bit immediates had to be changed, it
>   was not accepting unsigned 32-bit unsigned integers with the MSB
>   set. I believe const_int_operand expects the mode of the operand to be
>   set to VOIDmode and not SImode. I have only changed it in the patterns
>   that were affecting this code, though I suggest looking into changing
>   it in the rest of the ARM backend.
> 
> Thanks,
> Richard

This part of the patch was written by Andre. After checking with him, it seems
that some of the confusion arises from the comment on real_to_target() which
states "There are always 32 bits in each long, no matter the size of the host
long". While this may imply the value is zero-extended on hosts with wider
longs, it seems like the value is always correctly sign-extended and thus
gen_int_mode() should be unnecessary.

As for why VOIDmode is used with the values casted to int, there is a reason
for why it is done this way to get it working but this has been long-forgotten.
I only have the code and this message to rely on.