aarch64: Do not alter value on a force_reg returned rtx expanding __jcvt

Message ID gkrpn6fh14u.fsf@arm.com
State New
Headers show
Series
  • aarch64: Do not alter value on a force_reg returned rtx expanding __jcvt
Related show

Commit Message

Andrea Corallo Sept. 21, 2020, 10:31 a.m.
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(-)

-- 
2.17.1

Comments

Richard Sandiford Sept. 21, 2020, 10:58 a.m. | #1
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:
Kyrylo Tkachov Sept. 21, 2020, 11 a.m. | #2
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:
Andrea Corallo Sept. 21, 2020, 12:13 p.m. | #3
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 Sept. 24, 2020, 9:50 a.m. | #4
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

Patch

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: