| Message ID | gkrpn6fh14u.fsf@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64: Do not alter value on a force_reg returned rtx expanding __jcvt | expand |
Andrea Corallo <andrea.corallo@arm.com> writes: > Hi all, > > From the `force_reg` description comment I see the returned register > should not be modified, thus IIUC should not be used as a GEN_FCN > target. > > Assuming my interpretation is correct this fix this case inside > `aarch64_general_expand_builtin` while expanding expanding the > `__jcvt` intrinsic. If is not the case please discard. Good catch. > Regtested and bootsraped on aarch64-linux-gnu. > > Andrea > > From 403ad66b8f9c108d7f38b406ed1afcb603b7e25f Mon Sep 17 00:00:00 2001 > From: Andrea Corallo <andrea.corallo@arm.com> > Date: Thu, 17 Sep 2020 17:17:52 +0100 > Subject: [PATCH] aarch64: Do not alter value on a force_reg returned rtx > expanding __jcvt > > 2020-09-17 Andrea Corallo <andrea.corallo@arm.com> > > * config/aarch64/aarch64-builtins.c > (aarch64_general_expand_builtin): Use expand machinery not to > alter the value of an rtx returned by force_reg. OK, thanks. Richard > --- > gcc/config/aarch64/aarch64-builtins.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c > index 4f33dd936c7..b787719cf5e 100644 > --- a/gcc/config/aarch64/aarch64-builtins.c > +++ b/gcc/config/aarch64/aarch64-builtins.c > @@ -2128,14 +2128,14 @@ aarch64_general_expand_builtin (unsigned int fcode, tree exp, rtx target, > return target; > > case AARCH64_JSCVT: > - arg0 = CALL_EXPR_ARG (exp, 0); > - op0 = force_reg (DFmode, expand_normal (arg0)); > - if (!target) > - target = gen_reg_rtx (SImode); > - else > - target = force_reg (SImode, target); > - emit_insn (GEN_FCN (CODE_FOR_aarch64_fjcvtzs) (target, op0)); > - return target; > + { > + expand_operand ops[2]; > + create_output_operand (&ops[0], target, SImode); > + op0 = expand_normal (CALL_EXPR_ARG (exp, 0)); > + create_input_operand (&ops[1], op0, DFmode); > + expand_insn (CODE_FOR_aarch64_fjcvtzs, 2, ops); > + return ops[0].value; > + } > > case AARCH64_SIMD_BUILTIN_FCMLA_LANEQ0_V2SF: > case AARCH64_SIMD_BUILTIN_FCMLA_LANEQ90_V2SF:
Hi Andrea, > -----Original Message----- > From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of > Richard Sandiford > Sent: 21 September 2020 11:58 > To: Andrea Corallo <Andrea.Corallo@arm.com> > Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>; nd <nd@arm.com>; > gcc-patches@gcc.gnu.org > Subject: Re: [PATCH] aarch64: Do not alter value on a force_reg returned rtx > expanding __jcvt > > Andrea Corallo <andrea.corallo@arm.com> writes: > > Hi all, > > > > From the `force_reg` description comment I see the returned register > > should not be modified, thus IIUC should not be used as a GEN_FCN > > target. > > > > Assuming my interpretation is correct this fix this case inside > > `aarch64_general_expand_builtin` while expanding expanding the > > `__jcvt` intrinsic. If is not the case please discard. > > Good catch. Can you please also backport it to the appropriate branches as well after some time on trunk. Thanks, Kyrill > > > Regtested and bootsraped on aarch64-linux-gnu. > > > > Andrea > > > > From 403ad66b8f9c108d7f38b406ed1afcb603b7e25f Mon Sep 17 00:00:00 > 2001 > > From: Andrea Corallo <andrea.corallo@arm.com> > > Date: Thu, 17 Sep 2020 17:17:52 +0100 > > Subject: [PATCH] aarch64: Do not alter value on a force_reg returned rtx > > expanding __jcvt > > > > 2020-09-17 Andrea Corallo <andrea.corallo@arm.com> > > > > * config/aarch64/aarch64-builtins.c > > (aarch64_general_expand_builtin): Use expand machinery not to > > alter the value of an rtx returned by force_reg. > > OK, thanks. > > Richard > > > --- > > gcc/config/aarch64/aarch64-builtins.c | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/gcc/config/aarch64/aarch64-builtins.c > b/gcc/config/aarch64/aarch64-builtins.c > > index 4f33dd936c7..b787719cf5e 100644 > > --- a/gcc/config/aarch64/aarch64-builtins.c > > +++ b/gcc/config/aarch64/aarch64-builtins.c > > @@ -2128,14 +2128,14 @@ aarch64_general_expand_builtin (unsigned int > fcode, tree exp, rtx target, > > return target; > > > > case AARCH64_JSCVT: > > - arg0 = CALL_EXPR_ARG (exp, 0); > > - op0 = force_reg (DFmode, expand_normal (arg0)); > > - if (!target) > > - target = gen_reg_rtx (SImode); > > - else > > - target = force_reg (SImode, target); > > - emit_insn (GEN_FCN (CODE_FOR_aarch64_fjcvtzs) (target, op0)); > > - return target; > > + { > > + expand_operand ops[2]; > > + create_output_operand (&ops[0], target, SImode); > > + op0 = expand_normal (CALL_EXPR_ARG (exp, 0)); > > + create_input_operand (&ops[1], op0, DFmode); > > + expand_insn (CODE_FOR_aarch64_fjcvtzs, 2, ops); > > + return ops[0].value; > > + } > > > > case AARCH64_SIMD_BUILTIN_FCMLA_LANEQ0_V2SF: > > case AARCH64_SIMD_BUILTIN_FCMLA_LANEQ90_V2SF:
Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> writes: > Hi Andrea, > >> -----Original Message----- >> From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of >> Richard Sandiford >> Sent: 21 September 2020 11:58 >> To: Andrea Corallo <Andrea.Corallo@arm.com> >> Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>; nd <nd@arm.com>; >> gcc-patches@gcc.gnu.org >> Subject: Re: [PATCH] aarch64: Do not alter value on a force_reg returned rtx >> expanding __jcvt >> >> Andrea Corallo <andrea.corallo@arm.com> writes: >> > Hi all, >> > >> > From the `force_reg` description comment I see the returned register >> > should not be modified, thus IIUC should not be used as a GEN_FCN >> > target. >> > >> > Assuming my interpretation is correct this fix this case inside >> > `aarch64_general_expand_builtin` while expanding expanding the >> > `__jcvt` intrinsic. If is not the case please discard. >> >> Good catch. > > Can you please also backport it to the appropriate branches as well after some time on trunk. > Thanks, > Kyrill Ciao Kyrill, Sure happy to do that. For now into trunk as 2c62952f816. Thanks Andrea
Andrea Corallo <andrea.corallo@arm.com> writes: > Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> writes: [...] >> >> Can you please also backport it to the appropriate branches as well after some time on trunk. >> Thanks, >> Kyrill > > Ciao Kyrill, > > Sure happy to do that. For now into trunk as 2c62952f816. Backported into gcc-10 as aa47c987340. Thanks Andrea
diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c index 4f33dd936c7..b787719cf5e 100644 --- a/gcc/config/aarch64/aarch64-builtins.c +++ b/gcc/config/aarch64/aarch64-builtins.c @@ -2128,14 +2128,14 @@ aarch64_general_expand_builtin (unsigned int fcode, tree exp, rtx target, return target; case AARCH64_JSCVT: - arg0 = CALL_EXPR_ARG (exp, 0); - op0 = force_reg (DFmode, expand_normal (arg0)); - if (!target) - target = gen_reg_rtx (SImode); - else - target = force_reg (SImode, target); - emit_insn (GEN_FCN (CODE_FOR_aarch64_fjcvtzs) (target, op0)); - return target; + { + expand_operand ops[2]; + create_output_operand (&ops[0], target, SImode); + op0 = expand_normal (CALL_EXPR_ARG (exp, 0)); + create_input_operand (&ops[1], op0, DFmode); + expand_insn (CODE_FOR_aarch64_fjcvtzs, 2, ops); + return ops[0].value; + } case AARCH64_SIMD_BUILTIN_FCMLA_LANEQ0_V2SF: case AARCH64_SIMD_BUILTIN_FCMLA_LANEQ90_V2SF:
Hi all, From the `force_reg` description comment I see the returned register should not be modified, thus IIUC should not be used as a GEN_FCN target. Assuming my interpretation is correct this fix this case inside `aarch64_general_expand_builtin` while expanding expanding the `__jcvt` intrinsic. If is not the case please discard. Regtested and bootsraped on aarch64-linux-gnu. Andrea From 403ad66b8f9c108d7f38b406ed1afcb603b7e25f Mon Sep 17 00:00:00 2001 From: Andrea Corallo <andrea.corallo@arm.com> Date: Thu, 17 Sep 2020 17:17:52 +0100 Subject: [PATCH] aarch64: Do not alter value on a force_reg returned rtx expanding __jcvt 2020-09-17 Andrea Corallo <andrea.corallo@arm.com> * config/aarch64/aarch64-builtins.c (aarch64_general_expand_builtin): Use expand machinery not to alter the value of an rtx returned by force_reg. --- gcc/config/aarch64/aarch64-builtins.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)