aarch64: Fix ICE on fpsr fpcr getters [PR96968]

Message ID gkrh7s0ia9w.fsf@arm.com
State New
Headers show
Series
  • aarch64: Fix ICE on fpsr fpcr getters [PR96968]
Related show

Commit Message

Andrea Corallo Sept. 14, 2020, 4:38 p.m.
Hi all,

I'd like to submit this patch to fix PR96968 and add the corresponding
test.

The fix makes sure the target of these getters is a register so the
insn can be pattern matched correctly.

Regtested and bootsraped on aarch64-linux-gnu.

Okay for trunk?

Thanks

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

Date: Mon, 14 Sep 2020 14:47:24 +0100
Subject: [PATCH] aarch64: Fix ICE on fpsr fpcr getters [PR96968]

gcc/ChangeLog

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

	PR target/96968
	* config/aarch64/aarch64-builtins.c
	(aarch64_expand_fpsr_fpcr_setter): Fix comment nit.
	(aarch64_expand_fpsr_fpcr_getter): New function, expand these
	getters and have the target in a register.
	(aarch64_general_expand_builtin): Make use of.

gcc/testsuite/ChangeLog

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

	PR target/96968
	* gcc.target/aarch64/pr96968.c: New test.
---
 gcc/config/aarch64/aarch64-builtins.c      | 24 ++++++++++++-------
 gcc/testsuite/gcc.target/aarch64/pr96968.c | 28 ++++++++++++++++++++++
 2 files changed, 43 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr96968.c

-- 
2.17.1

Comments

Richard Sandiford Sept. 16, 2020, 11:42 a.m. | #1
Andrea Corallo <andrea.corallo@arm.com> writes:
> @@ -2034,6 +2034,16 @@ aarch64_expand_fpsr_fpcr_setter (int unspec, machine_mode mode, tree exp)

>    emit_insn (gen_aarch64_set (unspec, mode, op));

>  }

>  

> +/* Expand a fpsr or fpcr getter (depending on UNSPEC) using MODE.

> +   Return the target.  */

> +static rtx

> +aarch64_expand_fpsr_fpcr_getter (int unspec, machine_mode mode)

> +{

> +  rtx target = gen_reg_rtx (mode);

> +  emit_insn (gen_aarch64_get (unspec, mode, target));

> +  return target;

> +}


I agree this is functionally correct, but if a valid target has
been given to the caller, it's generally better to use it.
So IMO it would be better to use the expand_insn machinery,
passing the original target to create_output_operand.

Thanks,
Richard

> +

>  /* Expand an expression EXP that calls built-in function FCODE,

>     with result going to TARGET if that's convenient.  IGNORE is true

>     if the result of the builtin is ignored.  */

> @@ -2048,26 +2058,22 @@ aarch64_general_expand_builtin (unsigned int fcode, tree exp, rtx target,

>    switch (fcode)

>      {

>      case AARCH64_BUILTIN_GET_FPCR:

> -      emit_insn (gen_aarch64_get (UNSPECV_GET_FPCR, SImode, target));

> -      return target;

> +      return aarch64_expand_fpsr_fpcr_getter (UNSPECV_GET_FPCR, SImode);

>      case AARCH64_BUILTIN_SET_FPCR:

>        aarch64_expand_fpsr_fpcr_setter (UNSPECV_SET_FPCR, SImode, exp);

>        return target;

>      case AARCH64_BUILTIN_GET_FPSR:

> -      emit_insn (gen_aarch64_get (UNSPECV_GET_FPSR, SImode, target));

> -      return target;

> +      return aarch64_expand_fpsr_fpcr_getter (UNSPECV_GET_FPSR, SImode);

>      case AARCH64_BUILTIN_SET_FPSR:

>        aarch64_expand_fpsr_fpcr_setter (UNSPECV_SET_FPSR, SImode, exp);

>        return target;

>      case AARCH64_BUILTIN_GET_FPCR64:

> -      emit_insn (gen_aarch64_get (UNSPECV_GET_FPCR, DImode, target));

> -      return target;

> +      return aarch64_expand_fpsr_fpcr_getter (UNSPECV_GET_FPCR, DImode);

>      case AARCH64_BUILTIN_SET_FPCR64:

>        aarch64_expand_fpsr_fpcr_setter (UNSPECV_SET_FPCR, DImode, exp);

>        return target;

>      case AARCH64_BUILTIN_GET_FPSR64:

> -      emit_insn (gen_aarch64_get (UNSPECV_GET_FPSR, DImode, target));

> -      return target;

> +      return aarch64_expand_fpsr_fpcr_getter (UNSPECV_GET_FPSR, DImode);

>      case AARCH64_BUILTIN_SET_FPSR64:

>        aarch64_expand_fpsr_fpcr_setter (UNSPECV_SET_FPSR, DImode, exp);

>        return target;

> diff --git a/gcc/testsuite/gcc.target/aarch64/pr96968.c b/gcc/testsuite/gcc.target/aarch64/pr96968.c

> new file mode 100644

> index 00000000000..21ffd955153

> --- /dev/null

> +++ b/gcc/testsuite/gcc.target/aarch64/pr96968.c

> @@ -0,0 +1,28 @@

> +/* { dg-options "-O1" } */

> +

> +void

> +fpsr_getter (void)

> +{

> +  unsigned int fpsr = __builtin_aarch64_get_fpsr ();

> +}

> +

> +void

> +fpsr64_getter (void)

> +{

> +  unsigned long fpsr = __builtin_aarch64_get_fpsr64 ();

> +}

> +

> +void

> +fpcr_getter (void)

> +{

> +  unsigned int fpcr = __builtin_aarch64_get_fpcr ();

> +}

> +

> +void

> +fpcr64_getter (void)

> +{

> +  unsigned long fpcr = __builtin_aarch64_get_fpcr64 ();

> +}

> +

> +/* { dg-final { scan-assembler-times {\tmrs\tx0, fpsr\n} 2 } } */

> +/* { dg-final { scan-assembler-times {\tmrs\tx0, fpcr\n} 2 } } */
Andrea Corallo Sept. 17, 2020, 3:49 p.m. | #2
Hi all,

second version of the patch here implementing the suggestion of using
create_output_operand and the expand_insn machinery.

Regtested and bootsraped on aarch64-linux-gnu.

Okay for trunk?

Thanks

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

Date: Mon, 14 Sep 2020 14:47:24 +0100
Subject: [PATCH] aarch64: Fix ICE on fpsr fpcr getters [PR96968]

gcc/ChangeLog

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

	PR target/96968
	* config/aarch64/aarch64-builtins.c
	(aarch64_expand_fpsr_fpcr_setter): Fix comment nit.
	(aarch64_expand_fpsr_fpcr_getter): New function, expand these
	getters using expand_insn machinery.
	(aarch64_general_expand_builtin): Make use of.

gcc/testsuite/ChangeLog

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

	PR target/96968
	* gcc.target/aarch64/pr96968.c: New test.
---
 gcc/config/aarch64/aarch64-builtins.c      | 30 +++++++++++++++-------
 gcc/testsuite/gcc.target/aarch64/pr96968.c | 28 ++++++++++++++++++++
 2 files changed, 49 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr96968.c

diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
index 4f33dd936c7..7bb93f3b0cd 100644
--- a/gcc/config/aarch64/aarch64-builtins.c
+++ b/gcc/config/aarch64/aarch64-builtins.c
@@ -2024,7 +2024,7 @@ aarch64_expand_builtin_memtag (int fcode, tree exp, rtx target)
   return target;
 }
 
-/* Expand an expression EXP as fpsr or cpsr setter (depending on
+/* Expand an expression EXP as fpsr or fpcr setter (depending on
    UNSPEC) using MODE.  */
 static void
 aarch64_expand_fpsr_fpcr_setter (int unspec, machine_mode mode, tree exp)
@@ -2034,6 +2034,18 @@ aarch64_expand_fpsr_fpcr_setter (int unspec, machine_mode mode, tree exp)
   emit_insn (gen_aarch64_set (unspec, mode, op));
 }
 
+/* Expand a fpsr or fpcr getter (depending on UNSPEC) using MODE.
+   Return the target.  */
+static rtx
+aarch64_expand_fpsr_fpcr_getter (enum insn_code icode, machine_mode mode,
+				 rtx target)
+{
+  expand_operand op;
+  create_output_operand (&op, target, mode);
+  expand_insn (icode, 1, &op);
+  return target;
+}
+
 /* Expand an expression EXP that calls built-in function FCODE,
    with result going to TARGET if that's convenient.  IGNORE is true
    if the result of the builtin is ignored.  */
@@ -2048,26 +2060,26 @@ aarch64_general_expand_builtin (unsigned int fcode, tree exp, rtx target,
   switch (fcode)
     {
     case AARCH64_BUILTIN_GET_FPCR:
-      emit_insn (gen_aarch64_get (UNSPECV_GET_FPCR, SImode, target));
-      return target;
+      return aarch64_expand_fpsr_fpcr_getter (CODE_FOR_aarch64_get_fpcrsi,
+					      SImode, target);
     case AARCH64_BUILTIN_SET_FPCR:
       aarch64_expand_fpsr_fpcr_setter (UNSPECV_SET_FPCR, SImode, exp);
       return target;
     case AARCH64_BUILTIN_GET_FPSR:
-      emit_insn (gen_aarch64_get (UNSPECV_GET_FPSR, SImode, target));
-      return target;
+      return aarch64_expand_fpsr_fpcr_getter (CODE_FOR_aarch64_get_fpsrsi,
+					      SImode, target);
     case AARCH64_BUILTIN_SET_FPSR:
       aarch64_expand_fpsr_fpcr_setter (UNSPECV_SET_FPSR, SImode, exp);
       return target;
     case AARCH64_BUILTIN_GET_FPCR64:
-      emit_insn (gen_aarch64_get (UNSPECV_GET_FPCR, DImode, target));
-      return target;
+      return aarch64_expand_fpsr_fpcr_getter (CODE_FOR_aarch64_get_fpcrdi,
+					      DImode, target);
     case AARCH64_BUILTIN_SET_FPCR64:
       aarch64_expand_fpsr_fpcr_setter (UNSPECV_SET_FPCR, DImode, exp);
       return target;
     case AARCH64_BUILTIN_GET_FPSR64:
-      emit_insn (gen_aarch64_get (UNSPECV_GET_FPSR, DImode, target));
-      return target;
+      return aarch64_expand_fpsr_fpcr_getter (CODE_FOR_aarch64_get_fpsrdi,
+					      DImode, target);
     case AARCH64_BUILTIN_SET_FPSR64:
       aarch64_expand_fpsr_fpcr_setter (UNSPECV_SET_FPSR, DImode, exp);
       return target;
diff --git a/gcc/testsuite/gcc.target/aarch64/pr96968.c b/gcc/testsuite/gcc.target/aarch64/pr96968.c
new file mode 100644
index 00000000000..21ffd955153
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr96968.c
@@ -0,0 +1,28 @@
+/* { dg-options "-O1" } */
+
+void
+fpsr_getter (void)
+{
+  unsigned int fpsr = __builtin_aarch64_get_fpsr ();
+}
+
+void
+fpsr64_getter (void)
+{
+  unsigned long fpsr = __builtin_aarch64_get_fpsr64 ();
+}
+
+void
+fpcr_getter (void)
+{
+  unsigned int fpcr = __builtin_aarch64_get_fpcr ();
+}
+
+void
+fpcr64_getter (void)
+{
+  unsigned long fpcr = __builtin_aarch64_get_fpcr64 ();
+}
+
+/* { dg-final { scan-assembler-times {\tmrs\tx0, fpsr\n} 2 } } */
+/* { dg-final { scan-assembler-times {\tmrs\tx0, fpcr\n} 2 } } */
-- 
2.20.1
Richard Sandiford Sept. 17, 2020, 4:53 p.m. | #3
Andrea Corallo <andrea.corallo@arm.com> writes:
> Hi all,

>

> second version of the patch here implementing the suggestion of using

> create_output_operand and the expand_insn machinery.

>

> Regtested and bootsraped on aarch64-linux-gnu.

>

> Okay for trunk?

>

> Thanks

>

>   Andrea

>

> From bb35b56810f908c575fec11435071d1c37f6f1a3 Mon Sep 17 00:00:00 2001

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

> Date: Mon, 14 Sep 2020 14:47:24 +0100

> Subject: [PATCH] aarch64: Fix ICE on fpsr fpcr getters [PR96968]

>

> gcc/ChangeLog

>

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

>

> 	PR target/96968

> 	* config/aarch64/aarch64-builtins.c

> 	(aarch64_expand_fpsr_fpcr_setter): Fix comment nit.

> 	(aarch64_expand_fpsr_fpcr_getter): New function, expand these

> 	getters using expand_insn machinery.

> 	(aarch64_general_expand_builtin): Make use of.

>

> gcc/testsuite/ChangeLog

>

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

>

> 	PR target/96968

> 	* gcc.target/aarch64/pr96968.c: New test.

> ---

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

>  gcc/testsuite/gcc.target/aarch64/pr96968.c | 28 ++++++++++++++++++++

>  2 files changed, 49 insertions(+), 9 deletions(-)

>  create mode 100644 gcc/testsuite/gcc.target/aarch64/pr96968.c

>

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

> index 4f33dd936c7..7bb93f3b0cd 100644

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

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

> @@ -2024,7 +2024,7 @@ aarch64_expand_builtin_memtag (int fcode, tree exp, rtx target)

>    return target;

>  }

>  

> -/* Expand an expression EXP as fpsr or cpsr setter (depending on

> +/* Expand an expression EXP as fpsr or fpcr setter (depending on

>     UNSPEC) using MODE.  */

>  static void

>  aarch64_expand_fpsr_fpcr_setter (int unspec, machine_mode mode, tree exp)

> @@ -2034,6 +2034,18 @@ aarch64_expand_fpsr_fpcr_setter (int unspec, machine_mode mode, tree exp)

>    emit_insn (gen_aarch64_set (unspec, mode, op));

>  }

>  

> +/* Expand a fpsr or fpcr getter (depending on UNSPEC) using MODE.

> +   Return the target.  */

> +static rtx

> +aarch64_expand_fpsr_fpcr_getter (enum insn_code icode, machine_mode mode,

> +				 rtx target)

> +{

> +  expand_operand op;

> +  create_output_operand (&op, target, mode);

> +  expand_insn (icode, 1, &op);

> +  return target;


This needs to be:

  return op[0].value;

so that we use whatever target the expand machinery chose.

OK with that change, thanks.

Richard
Richard Sandiford Sept. 17, 2020, 4:56 p.m. | #4
Richard Sandiford <richard.sandiford@arm.com> writes:
>> @@ -2034,6 +2034,18 @@ aarch64_expand_fpsr_fpcr_setter (int unspec, machine_mode mode, tree exp)

>>    emit_insn (gen_aarch64_set (unspec, mode, op));

>>  }

>>  

>> +/* Expand a fpsr or fpcr getter (depending on UNSPEC) using MODE.

>> +   Return the target.  */

>> +static rtx

>> +aarch64_expand_fpsr_fpcr_getter (enum insn_code icode, machine_mode mode,

>> +				 rtx target)

>> +{

>> +  expand_operand op;

>> +  create_output_operand (&op, target, mode);

>> +  expand_insn (icode, 1, &op);

>> +  return target;

>

> This needs to be:

>

>   return op[0].value;


Er, of course I mean op.value.  Muscle memory, sorry. :-)

>

> so that we use whatever target the expand machinery chose.

>

> OK with that change, thanks.

>

> Richard
Andrea Corallo Sept. 21, 2020, 8:42 a.m. | #5
Richard Sandiford <richard.sandiford@arm.com> writes:

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

>>> @@ -2034,6 +2034,18 @@ aarch64_expand_fpsr_fpcr_setter (int unspec, machine_mode mode, tree exp)

>>>    emit_insn (gen_aarch64_set (unspec, mode, op));

>>>  }

>>>  

>>> +/* Expand a fpsr or fpcr getter (depending on UNSPEC) using MODE.

>>> +   Return the target.  */

>>> +static rtx

>>> +aarch64_expand_fpsr_fpcr_getter (enum insn_code icode, machine_mode mode,

>>> +				 rtx target)

>>> +{

>>> +  expand_operand op;

>>> +  create_output_operand (&op, target, mode);

>>> +  expand_insn (icode, 1, &op);

>>> +  return target;

>>

>> This needs to be:

>>

>>   return op[0].value;

>

> Er, of course I mean op.value.  Muscle memory, sorry. :-)

>

>>

>> so that we use whatever target the expand machinery chose.

>>

>> OK with that change, thanks.

>>

>> Richard


Installed in trunk as f5e73de00e9.

Thanks!

  Andrea

Patch

diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
index 4f33dd936c7..f640a9e5de1 100644
--- a/gcc/config/aarch64/aarch64-builtins.c
+++ b/gcc/config/aarch64/aarch64-builtins.c
@@ -2024,7 +2024,7 @@  aarch64_expand_builtin_memtag (int fcode, tree exp, rtx target)
   return target;
 }
 
-/* Expand an expression EXP as fpsr or cpsr setter (depending on
+/* Expand an expression EXP as fpsr or fpcr setter (depending on
    UNSPEC) using MODE.  */
 static void
 aarch64_expand_fpsr_fpcr_setter (int unspec, machine_mode mode, tree exp)
@@ -2034,6 +2034,16 @@  aarch64_expand_fpsr_fpcr_setter (int unspec, machine_mode mode, tree exp)
   emit_insn (gen_aarch64_set (unspec, mode, op));
 }
 
+/* Expand a fpsr or fpcr getter (depending on UNSPEC) using MODE.
+   Return the target.  */
+static rtx
+aarch64_expand_fpsr_fpcr_getter (int unspec, machine_mode mode)
+{
+  rtx target = gen_reg_rtx (mode);
+  emit_insn (gen_aarch64_get (unspec, mode, target));
+  return target;
+}
+
 /* Expand an expression EXP that calls built-in function FCODE,
    with result going to TARGET if that's convenient.  IGNORE is true
    if the result of the builtin is ignored.  */
@@ -2048,26 +2058,22 @@  aarch64_general_expand_builtin (unsigned int fcode, tree exp, rtx target,
   switch (fcode)
     {
     case AARCH64_BUILTIN_GET_FPCR:
-      emit_insn (gen_aarch64_get (UNSPECV_GET_FPCR, SImode, target));
-      return target;
+      return aarch64_expand_fpsr_fpcr_getter (UNSPECV_GET_FPCR, SImode);
     case AARCH64_BUILTIN_SET_FPCR:
       aarch64_expand_fpsr_fpcr_setter (UNSPECV_SET_FPCR, SImode, exp);
       return target;
     case AARCH64_BUILTIN_GET_FPSR:
-      emit_insn (gen_aarch64_get (UNSPECV_GET_FPSR, SImode, target));
-      return target;
+      return aarch64_expand_fpsr_fpcr_getter (UNSPECV_GET_FPSR, SImode);
     case AARCH64_BUILTIN_SET_FPSR:
       aarch64_expand_fpsr_fpcr_setter (UNSPECV_SET_FPSR, SImode, exp);
       return target;
     case AARCH64_BUILTIN_GET_FPCR64:
-      emit_insn (gen_aarch64_get (UNSPECV_GET_FPCR, DImode, target));
-      return target;
+      return aarch64_expand_fpsr_fpcr_getter (UNSPECV_GET_FPCR, DImode);
     case AARCH64_BUILTIN_SET_FPCR64:
       aarch64_expand_fpsr_fpcr_setter (UNSPECV_SET_FPCR, DImode, exp);
       return target;
     case AARCH64_BUILTIN_GET_FPSR64:
-      emit_insn (gen_aarch64_get (UNSPECV_GET_FPSR, DImode, target));
-      return target;
+      return aarch64_expand_fpsr_fpcr_getter (UNSPECV_GET_FPSR, DImode);
     case AARCH64_BUILTIN_SET_FPSR64:
       aarch64_expand_fpsr_fpcr_setter (UNSPECV_SET_FPSR, DImode, exp);
       return target;
diff --git a/gcc/testsuite/gcc.target/aarch64/pr96968.c b/gcc/testsuite/gcc.target/aarch64/pr96968.c
new file mode 100644
index 00000000000..21ffd955153
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr96968.c
@@ -0,0 +1,28 @@ 
+/* { dg-options "-O1" } */
+
+void
+fpsr_getter (void)
+{
+  unsigned int fpsr = __builtin_aarch64_get_fpsr ();
+}
+
+void
+fpsr64_getter (void)
+{
+  unsigned long fpsr = __builtin_aarch64_get_fpsr64 ();
+}
+
+void
+fpcr_getter (void)
+{
+  unsigned int fpcr = __builtin_aarch64_get_fpcr ();
+}
+
+void
+fpcr64_getter (void)
+{
+  unsigned long fpcr = __builtin_aarch64_get_fpcr64 ();
+}
+
+/* { dg-final { scan-assembler-times {\tmrs\tx0, fpsr\n} 2 } } */
+/* { dg-final { scan-assembler-times {\tmrs\tx0, fpcr\n} 2 } } */