aarch64: Do not alter force_reg returned rtx expanding pauth builtins

Message ID gkrlfh2gmdk.fsf@arm.com
State New
Headers show
Series
  • aarch64: Do not alter force_reg returned rtx expanding pauth builtins
Related show

Commit Message

Andrea Corallo Sept. 22, 2020, 10:02 a.m.
Hi all,

having a look for force_reg returned rtx later on modified I've found
this other case in `aarch64_general_expand_builtin` while expanding 
pointer authentication builtins.

Regtested and bootsraped on aarch64-linux-gnu.

Okay for trunk?

  Andrea
From 8869ee04e3788fdec86aa7e5a13e2eb477091d0e Mon Sep 17 00:00:00 2001
From: Andrea Corallo <andrea.corallo@arm.com>

Date: Mon, 21 Sep 2020 13:52:45 +0100
Subject: [PATCH] aarch64: Do not alter force_reg returned rtx expanding pauth
 builtins

2020-09-21  Andrea Corallo  <andrea.corallo@arm.com>

	* config/aarch64/aarch64-builtins.c
	(aarch64_general_expand_builtin): Do not alter value on a
	force_reg returned rtx.
---
 gcc/config/aarch64/aarch64-builtins.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.17.1

Comments

Richard Sandiford Sept. 23, 2020, 10:33 a.m. | #1
Andrea Corallo <andrea.corallo@arm.com> writes:
> Hi all,

>

> having a look for force_reg returned rtx later on modified I've found

> this other case in `aarch64_general_expand_builtin` while expanding 

> pointer authentication builtins.

>

> Regtested and bootsraped on aarch64-linux-gnu.

>

> Okay for trunk?

>

>   Andrea

>

> From 8869ee04e3788fdec86aa7e5a13e2eb477091d0e Mon Sep 17 00:00:00 2001

> From: Andrea Corallo <andrea.corallo@arm.com>

> Date: Mon, 21 Sep 2020 13:52:45 +0100

> Subject: [PATCH] aarch64: Do not alter force_reg returned rtx expanding pauth

>  builtins

>

> 2020-09-21  Andrea Corallo  <andrea.corallo@arm.com>

>

> 	* config/aarch64/aarch64-builtins.c

> 	(aarch64_general_expand_builtin): Do not alter value on a

> 	force_reg returned rtx.

> ---

>  gcc/config/aarch64/aarch64-builtins.c | 6 +++---

>  1 file changed, 3 insertions(+), 3 deletions(-)

>

> diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c

> index b787719cf5e..a77718ccfac 100644

> --- a/gcc/config/aarch64/aarch64-builtins.c

> +++ b/gcc/config/aarch64/aarch64-builtins.c

> @@ -2079,10 +2079,10 @@ aarch64_general_expand_builtin (unsigned int fcode, tree exp, rtx target,

>        arg0 = CALL_EXPR_ARG (exp, 0);

>        op0 = force_reg (Pmode, expand_normal (arg0));

>  

> -      if (!target)

> +      if (!(target

> +	    && REG_P (target)

> +	    && GET_MODE (target) == Pmode))

>  	target = gen_reg_rtx (Pmode);

> -      else

> -	target = force_reg (Pmode, target);

>  

>        emit_move_insn (target, op0);


Do we actually use the result of this move?  It looked like we always
use op0 rather than target (good) and overwrite target with a later move.

If so, I think we should delete the move and convert the later code
to use expand_insn.

Thanks,
Richard
Andrea Corallo Sept. 24, 2020, 3:20 p.m. | #2
Hi Richard,

thanks for reviewing

Richard Sandiford <richard.sandiford@arm.com> writes:

> Andrea Corallo <andrea.corallo@arm.com> writes:

>> Hi all,

>>

>> having a look for force_reg returned rtx later on modified I've found

>> this other case in `aarch64_general_expand_builtin` while expanding 

>> pointer authentication builtins.

>>

>> Regtested and bootsraped on aarch64-linux-gnu.

>>

>> Okay for trunk?

>>

>>   Andrea

>>

>> From 8869ee04e3788fdec86aa7e5a13e2eb477091d0e Mon Sep 17 00:00:00 2001

>> From: Andrea Corallo <andrea.corallo@arm.com>

>> Date: Mon, 21 Sep 2020 13:52:45 +0100

>> Subject: [PATCH] aarch64: Do not alter force_reg returned rtx expanding pauth

>>  builtins

>>

>> 2020-09-21  Andrea Corallo  <andrea.corallo@arm.com>

>>

>> 	* config/aarch64/aarch64-builtins.c

>> 	(aarch64_general_expand_builtin): Do not alter value on a

>> 	force_reg returned rtx.

>> ---

>>  gcc/config/aarch64/aarch64-builtins.c | 6 +++---

>>  1 file changed, 3 insertions(+), 3 deletions(-)

>>

>> diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c

>> index b787719cf5e..a77718ccfac 100644

>> --- a/gcc/config/aarch64/aarch64-builtins.c

>> +++ b/gcc/config/aarch64/aarch64-builtins.c

>> @@ -2079,10 +2079,10 @@ aarch64_general_expand_builtin (unsigned int fcode, tree exp, rtx target,

>>        arg0 = CALL_EXPR_ARG (exp, 0);

>>        op0 = force_reg (Pmode, expand_normal (arg0));

>>  

>> -      if (!target)

>> +      if (!(target

>> +	    && REG_P (target)

>> +	    && GET_MODE (target) == Pmode))

>>  	target = gen_reg_rtx (Pmode);

>> -      else

>> -	target = force_reg (Pmode, target);

>>  

>>        emit_move_insn (target, op0);

>

> Do we actually use the result of this move?  It looked like we always

> use op0 rather than target (good) and overwrite target with a later move.

>

> If so, I think we should delete the move


Good point agree.

> and convert the later code to use expand_insn.


I'm not sure I understand the suggestion right, xpaclri&friends patterns
are written with hardcoded in/out regs, is the suggestion to just use like
'expand_insn (CODE_FOR_xpaclri, 0, NULL)' in place of GEN_FCN+emit_insn?

Thanks!

  Andrea
Richard Sandiford Sept. 25, 2020, 12:27 p.m. | #3
Andrea Corallo <andrea.corallo@arm.com> writes:
> Hi Richard,

>

> thanks for reviewing

>

> Richard Sandiford <richard.sandiford@arm.com> writes:

>

>> Andrea Corallo <andrea.corallo@arm.com> writes:

>>> Hi all,

>>>

>>> having a look for force_reg returned rtx later on modified I've found

>>> this other case in `aarch64_general_expand_builtin` while expanding 

>>> pointer authentication builtins.

>>>

>>> Regtested and bootsraped on aarch64-linux-gnu.

>>>

>>> Okay for trunk?

>>>

>>>   Andrea

>>>

>>> From 8869ee04e3788fdec86aa7e5a13e2eb477091d0e Mon Sep 17 00:00:00 2001

>>> From: Andrea Corallo <andrea.corallo@arm.com>

>>> Date: Mon, 21 Sep 2020 13:52:45 +0100

>>> Subject: [PATCH] aarch64: Do not alter force_reg returned rtx expanding pauth

>>>  builtins

>>>

>>> 2020-09-21  Andrea Corallo  <andrea.corallo@arm.com>

>>>

>>> 	* config/aarch64/aarch64-builtins.c

>>> 	(aarch64_general_expand_builtin): Do not alter value on a

>>> 	force_reg returned rtx.

>>> ---

>>>  gcc/config/aarch64/aarch64-builtins.c | 6 +++---

>>>  1 file changed, 3 insertions(+), 3 deletions(-)

>>>

>>> diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c

>>> index b787719cf5e..a77718ccfac 100644

>>> --- a/gcc/config/aarch64/aarch64-builtins.c

>>> +++ b/gcc/config/aarch64/aarch64-builtins.c

>>> @@ -2079,10 +2079,10 @@ aarch64_general_expand_builtin (unsigned int fcode, tree exp, rtx target,

>>>        arg0 = CALL_EXPR_ARG (exp, 0);

>>>        op0 = force_reg (Pmode, expand_normal (arg0));

>>>  

>>> -      if (!target)

>>> +      if (!(target

>>> +	    && REG_P (target)

>>> +	    && GET_MODE (target) == Pmode))

>>>  	target = gen_reg_rtx (Pmode);

>>> -      else

>>> -	target = force_reg (Pmode, target);

>>>  

>>>        emit_move_insn (target, op0);

>>

>> Do we actually use the result of this move?  It looked like we always

>> use op0 rather than target (good) and overwrite target with a later move.

>>

>> If so, I think we should delete the move

>

> Good point agree.

>

>> and convert the later code to use expand_insn.

>

> I'm not sure I understand the suggestion right, xpaclri&friends patterns

> are written with hardcoded in/out regs, is the suggestion to just use like

> 'expand_insn (CODE_FOR_xpaclri, 0, NULL)' in place of GEN_FCN+emit_insn?


Oops, sorry for the bogus comment, didn't look closely enough.

So yeah, no need to use expand_insn.  Rather than generate a new target,
it should be OK to return lr and x17_reg directly.  (Hope I'm right
this time. ;-))

Thanks,
Richard

Patch

diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
index b787719cf5e..a77718ccfac 100644
--- a/gcc/config/aarch64/aarch64-builtins.c
+++ b/gcc/config/aarch64/aarch64-builtins.c
@@ -2079,10 +2079,10 @@  aarch64_general_expand_builtin (unsigned int fcode, tree exp, rtx target,
       arg0 = CALL_EXPR_ARG (exp, 0);
       op0 = force_reg (Pmode, expand_normal (arg0));
 
-      if (!target)
+      if (!(target
+	    && REG_P (target)
+	    && GET_MODE (target) == Pmode))
 	target = gen_reg_rtx (Pmode);
-      else
-	target = force_reg (Pmode, target);
 
       emit_move_insn (target, op0);