| Message ID | 2ee02a7d9d6cbab11c23563cc9f561cdcbe8b9af.1532955615.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm: Generate correct const_ints (PR86640) | expand |
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 >
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
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
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
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
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
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
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);