[PATCH] arm: Generate correct const_ints (PR86640)

[PATCH] arm: Generate correct const_ints (PR86640)

[Segher Boessenkool]

In arm_block_set_aligned_vect 8-bit constants are generated as zero-
extended const_ints, not sign-extended as required.  Fix that.

Tamar tested the patch (see PR); no problems were found.  Is this okay
for trunk?


Segher


2018-07-30  Segher Boessenkool  <segher@kernel.crashing.org>

	PR target/86640
	* config/arm/arm.c (arm_block_set_aligned_vect): Use gen_int_mode
	instead of GEN_INT.

---
 gcc/config/arm/arm.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index cf12ace..f5eece4 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -30046,7 +30046,6 @@ arm_block_set_aligned_vect (rtx dstbase,
   rtx dst, addr, mem;
   rtx val_vec, reg;
   machine_mode mode;
-  unsigned HOST_WIDE_INT v = value;
   unsigned int offset = 0;
 
   gcc_assert ((align & 0x3) == 0);
@@ -30065,10 +30064,8 @@ arm_block_set_aligned_vect (rtx dstbase,
 
   dst = copy_addr_to_reg (XEXP (dstbase, 0));
 
-  v = sext_hwi (v, BITS_PER_WORD);
-
   reg = gen_reg_rtx (mode);
-  val_vec = gen_const_vec_duplicate (mode, GEN_INT (v));
+  val_vec = gen_const_vec_duplicate (mode, gen_int_mode (value, QImode));
   /* Emit instruction loading the constant value.  */
   emit_move_insn (reg, val_vec);
 
-- 
1.8.3.1

Re: [PATCH] arm: Generate correct const_ints (PR86640)

[Kyrill Tkachov]

Hi Segher,

On 30/07/18 14:14, Segher Boessenkool wrote:
> In arm_block_set_aligned_vect 8-bit constants are generated as zero-
> extended const_ints, not sign-extended as required.  Fix that.
>
> Tamar tested the patch (see PR); no problems were found.  Is this okay
> for trunk?
>

The patch is okay but please add the testcase from the PR to gcc.dg/
or somewhere else generic (it's not arm-specific).
Thanks Tamar for testing.

Kyrill

>
> Segher
>
>
> 2018-07-30  Segher Boessenkool <segher@kernel.crashing.org>
>
>         PR target/86640
>         * config/arm/arm.c (arm_block_set_aligned_vect): Use gen_int_mode
>         instead of GEN_INT.
>
> ---
>  gcc/config/arm/arm.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index cf12ace..f5eece4 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -30046,7 +30046,6 @@ arm_block_set_aligned_vect (rtx dstbase,
>    rtx dst, addr, mem;
>    rtx val_vec, reg;
>    machine_mode mode;
> -  unsigned HOST_WIDE_INT v = value;
>    unsigned int offset = 0;
>
>    gcc_assert ((align & 0x3) == 0);
> @@ -30065,10 +30064,8 @@ arm_block_set_aligned_vect (rtx dstbase,
>
>    dst = copy_addr_to_reg (XEXP (dstbase, 0));
>
> -  v = sext_hwi (v, BITS_PER_WORD);
> -
>    reg = gen_reg_rtx (mode);
> -  val_vec = gen_const_vec_duplicate (mode, GEN_INT (v));
> +  val_vec = gen_const_vec_duplicate (mode, gen_int_mode (value, QImode));
>    /* Emit instruction loading the constant value.  */
>    emit_move_insn (reg, val_vec);
>
> -- 
> 1.8.3.1
>

Re: [PATCH] arm: Generate correct const_ints (PR86640)

[Segher Boessenkool]

On Mon, Jul 30, 2018 at 03:55:30PM +0100, Kyrill Tkachov wrote:
> Hi Segher,
> 
> On 30/07/18 14:14, Segher Boessenkool wrote:
> >In arm_block_set_aligned_vect 8-bit constants are generated as zero-
> >extended const_ints, not sign-extended as required.  Fix that.
> >
> >Tamar tested the patch (see PR); no problems were found.  Is this okay
> >for trunk?
> >
> 
> The patch is okay but please add the testcase from the PR to gcc.dg/
> or somewhere else generic (it's not arm-specific).

It only failed with very specific options, which aren't valid on every
ARM config either I think?

-O3 -mfpu=neon -mfloat-abi=hard -march=armv7-a

I don't know the magic incantations for ARM tests, sorry.


Segher

Re: [PATCH] arm: Generate correct const_ints (PR86640)

[Kyrill Tkachov]

Hi Segher,

On 30/07/18 18:37, Segher Boessenkool wrote:
> On Mon, Jul 30, 2018 at 03:55:30PM +0100, Kyrill Tkachov wrote:
>> Hi Segher,
>>
>> On 30/07/18 14:14, Segher Boessenkool wrote:
>>> In arm_block_set_aligned_vect 8-bit constants are generated as zero-
>>> extended const_ints, not sign-extended as required.  Fix that.
>>>
>>> Tamar tested the patch (see PR); no problems were found.  Is this okay
>>> for trunk?
>>>
>> The patch is okay but please add the testcase from the PR to gcc.dg/
>> or somewhere else generic (it's not arm-specific).
> It only failed with very specific options, which aren't valid on every
> ARM config either I think?
>
> -O3 -mfpu=neon -mfloat-abi=hard -march=armv7-a

This is a fairly common Arm configuration that is tested by many people by default,
so you wouldn't need to add any arm-specific directives. Just putting it in one of
the generic C tests folders would be enough.

Thanks for fixing this,
Kyrill

> I don't know the magic incantations for ARM tests, sorry.
>
>
> Segher

Re: [PATCH] arm: Generate correct const_ints (PR86640)

[Segher Boessenkool]

On Tue, Jul 31, 2018 at 09:02:56AM +0100, Kyrill Tkachov wrote:
> Hi Segher,
> 
> On 30/07/18 18:37, Segher Boessenkool wrote:
> >On Mon, Jul 30, 2018 at 03:55:30PM +0100, Kyrill Tkachov wrote:
> >>Hi Segher,
> >>
> >>On 30/07/18 14:14, Segher Boessenkool wrote:
> >>>In arm_block_set_aligned_vect 8-bit constants are generated as zero-
> >>>extended const_ints, not sign-extended as required.  Fix that.
> >>>
> >>>Tamar tested the patch (see PR); no problems were found.  Is this okay
> >>>for trunk?
> >>>
> >>The patch is okay but please add the testcase from the PR to gcc.dg/
> >>or somewhere else generic (it's not arm-specific).
> >It only failed with very specific options, which aren't valid on every
> >ARM config either I think?
> >
> >-O3 -mfpu=neon -mfloat-abi=hard -march=armv7-a
> 
> This is a fairly common Arm configuration that is tested by many people by 
> default,
> so you wouldn't need to add any arm-specific directives. Just putting it in 
> one of
> the generic C tests folders would be enough.

Ah ok, so just dg-options -O3 will do?

(As before, I have no reasonable way to test this).


Segher

Re: [PATCH] arm: Generate correct const_ints (PR86640)

[Kyrill Tkachov]

On 31/07/18 12:38, Segher Boessenkool wrote:
> On Tue, Jul 31, 2018 at 09:02:56AM +0100, Kyrill Tkachov wrote:
>> Hi Segher,
>>
>> On 30/07/18 18:37, Segher Boessenkool wrote:
>>> On Mon, Jul 30, 2018 at 03:55:30PM +0100, Kyrill Tkachov wrote:
>>>> Hi Segher,
>>>>
>>>> On 30/07/18 14:14, Segher Boessenkool wrote:
>>>>> In arm_block_set_aligned_vect 8-bit constants are generated as zero-
>>>>> extended const_ints, not sign-extended as required.  Fix that.
>>>>>
>>>>> Tamar tested the patch (see PR); no problems were found.  Is this okay
>>>>> for trunk?
>>>>>
>>>> The patch is okay but please add the testcase from the PR to gcc.dg/
>>>> or somewhere else generic (it's not arm-specific).
>>> It only failed with very specific options, which aren't valid on every
>>> ARM config either I think?
>>>
>>> -O3 -mfpu=neon -mfloat-abi=hard -march=armv7-a
>> This is a fairly common Arm configuration that is tested by many people by
>> default,
>> so you wouldn't need to add any arm-specific directives. Just putting it in
>> one of
>> the generic C tests folders would be enough.
> Ah ok, so just dg-options -O3 will do?

yes, that should be enough.

Thanks,
Kyrill

> (As before, I have no reasonable way to test this).
>
>
> Segher

RE: [PATCH] arm: Generate correct const_ints (PR86640)

[Tamar Christina]

Hi All,

I'd like to ask for permissions to backport of this patch to GCC 8?

Thanks,
Tamar

> -----Original Message-----
> From: Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>
> Sent: Tuesday, July 31, 2018 12:55
> To: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: gcc-patches@gcc.gnu.org; tnfchris@gcc.gnu.org
> Subject: Re: [PATCH] arm: Generate correct const_ints (PR86640)
> 
> 
> On 31/07/18 12:38, Segher Boessenkool wrote:
> > On Tue, Jul 31, 2018 at 09:02:56AM +0100, Kyrill Tkachov wrote:
> >> Hi Segher,
> >>
> >> On 30/07/18 18:37, Segher Boessenkool wrote:
> >>> On Mon, Jul 30, 2018 at 03:55:30PM +0100, Kyrill Tkachov wrote:
> >>>> Hi Segher,
> >>>>
> >>>> On 30/07/18 14:14, Segher Boessenkool wrote:
> >>>>> In arm_block_set_aligned_vect 8-bit constants are generated as
> >>>>> zero- extended const_ints, not sign-extended as required.  Fix that.
> >>>>>
> >>>>> Tamar tested the patch (see PR); no problems were found.  Is this
> >>>>> okay for trunk?
> >>>>>
> >>>> The patch is okay but please add the testcase from the PR to
> >>>> gcc.dg/ or somewhere else generic (it's not arm-specific).
> >>> It only failed with very specific options, which aren't valid on
> >>> every ARM config either I think?
> >>>
> >>> -O3 -mfpu=neon -mfloat-abi=hard -march=armv7-a
> >> This is a fairly common Arm configuration that is tested by many
> >> people by default, so you wouldn't need to add any arm-specific
> >> directives. Just putting it in one of the generic C tests folders
> >> would be enough.
> > Ah ok, so just dg-options -O3 will do?
> 
> yes, that should be enough.
> 
> Thanks,
> Kyrill
> 
> > (As before, I have no reasonable way to test this).
> >
> >
> > Segher

Re: [PATCH] arm: Generate correct const_ints (PR86640)

[Kyrill Tkachov]

On 15/08/18 09:21, Tamar Christina wrote:
> Hi All,
>
> I'd like to ask for permissions to backport of this patch to GCC 8?

Ok. Please bundle the testcase in the backport as well.

Thanks,
Kyrill

> Thanks,
> Tamar
>
>> -----Original Message-----
>> From: Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>
>> Sent: Tuesday, July 31, 2018 12:55
>> To: Segher Boessenkool <segher@kernel.crashing.org>
>> Cc: gcc-patches@gcc.gnu.org; tnfchris@gcc.gnu.org
>> Subject: Re: [PATCH] arm: Generate correct const_ints (PR86640)
>>
>>
>> On 31/07/18 12:38, Segher Boessenkool wrote:
>>> On Tue, Jul 31, 2018 at 09:02:56AM +0100, Kyrill Tkachov wrote:
>>>> Hi Segher,
>>>>
>>>> On 30/07/18 18:37, Segher Boessenkool wrote:
>>>>> On Mon, Jul 30, 2018 at 03:55:30PM +0100, Kyrill Tkachov wrote:
>>>>>> Hi Segher,
>>>>>>
>>>>>> On 30/07/18 14:14, Segher Boessenkool wrote:
>>>>>>> In arm_block_set_aligned_vect 8-bit constants are generated as
>>>>>>> zero- extended const_ints, not sign-extended as required.  Fix that.
>>>>>>>
>>>>>>> Tamar tested the patch (see PR); no problems were found.  Is this
>>>>>>> okay for trunk?
>>>>>>>
>>>>>> The patch is okay but please add the testcase from the PR to
>>>>>> gcc.dg/ or somewhere else generic (it's not arm-specific).
>>>>> It only failed with very specific options, which aren't valid on
>>>>> every ARM config either I think?
>>>>>
>>>>> -O3 -mfpu=neon -mfloat-abi=hard -march=armv7-a
>>>> This is a fairly common Arm configuration that is tested by many
>>>> people by default, so you wouldn't need to add any arm-specific
>>>> directives. Just putting it in one of the generic C tests folders
>>>> would be enough.
>>> Ah ok, so just dg-options -O3 will do?
>> yes, that should be enough.
>>
>> Thanks,
>> Kyrill
>>
>>> (As before, I have no reasonable way to test this).
>>>
>>>
>>> Segher