diff mbox series

i386: Fix splitters that call extract_insn_cached [PR93611]

Message ID 20200207075806.GQ17695@tucnak
State New
Headers show
Series i386: Fix splitters that call extract_insn_cached [PR93611] | expand

Commit Message

Jakub Jelinek Feb. 7, 2020, 7:58 a.m. UTC
Hi!

The following testcase ICEs.  The generated split_insns starts
with recog_data.insn = NULL and then tries to put various operands into
recog_data.operand array and checks various splitter conditions.
The problem is that some atom related tuning splitters indirectly call
extract_insn_cached on the insn they are used in.  This can change
recog_data.operand, but most likely it will just keep it as is, but
sets recog_data.insn to the current instruction.  If that splitter doesn't
match, we continue trying some other split conditions and modify
recog_data.operand array again.  If even that doesn't find any usable
splitter, we punt, but at that point recog_data.insn says that recog_data
is valid for that particular instruction, even when recog_data.operand array
can be anything.
The safest thing would be to copy whole recog_data to a temporary object
before doing the calls that can call extract_insn_cached and restore it
afterwards, but it would be also very costly, recog_data has 1280 bytes.
So, this patch just makes sure to clear recog_data.insn if it has changed
during the extract_insn_cached call, which means if we extract_insn_cached
later, we'll extract it properly, while if we call it say from some other
context than splitter conditions, the insn is already cached, we don't reset
the cache.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2020-02-07  Jakub Jelinek  <jakub@redhat.com>

	PR target/93611
	* config/i386/i386.c (ix86_lea_outperforms): Make sure to clear
	recog_data.insn if distance_non_agu_define changed it.

	* gcc.target/i386/pr93611.c: New test.


	Jakub

Comments

Uros Bizjak Feb. 7, 2020, 8:02 a.m. UTC | #1
On Fri, Feb 7, 2020 at 8:58 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> The following testcase ICEs.  The generated split_insns starts
> with recog_data.insn = NULL and then tries to put various operands into
> recog_data.operand array and checks various splitter conditions.
> The problem is that some atom related tuning splitters indirectly call
> extract_insn_cached on the insn they are used in.  This can change
> recog_data.operand, but most likely it will just keep it as is, but
> sets recog_data.insn to the current instruction.  If that splitter doesn't
> match, we continue trying some other split conditions and modify
> recog_data.operand array again.  If even that doesn't find any usable
> splitter, we punt, but at that point recog_data.insn says that recog_data
> is valid for that particular instruction, even when recog_data.operand array
> can be anything.
> The safest thing would be to copy whole recog_data to a temporary object
> before doing the calls that can call extract_insn_cached and restore it
> afterwards, but it would be also very costly, recog_data has 1280 bytes.
> So, this patch just makes sure to clear recog_data.insn if it has changed
> during the extract_insn_cached call, which means if we extract_insn_cached
> later, we'll extract it properly, while if we call it say from some other
> context than splitter conditions, the insn is already cached, we don't reset
> the cache.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2020-02-07  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/93611
>         * config/i386/i386.c (ix86_lea_outperforms): Make sure to clear
>         recog_data.insn if distance_non_agu_define changed it.
>
>         * gcc.target/i386/pr93611.c: New test.

LGTM.

Thanks,
Uros.

> --- gcc/config/i386/i386.c.jj   2020-01-28 08:45:56.781090684 +0100
> +++ gcc/config/i386/i386.c      2020-02-06 16:29:35.548663197 +0100
> @@ -14459,9 +14459,18 @@ ix86_lea_outperforms (rtx_insn *insn, un
>        return true;
>      }
>
> +  rtx_insn *rinsn = recog_data.insn;
> +
>    dist_define = distance_non_agu_define (regno1, regno2, insn);
>    dist_use = distance_agu_use (regno0, insn);
>
> +  /* distance_non_agu_define can call extract_insn_cached.  If this function
> +     is called from define_split conditions, that can break insn splitting,
> +     because split_insns works by clearing recog_data.insn and then modifying
> +     recog_data.operand array and match the various split conditions.  */
> +  if (recog_data.insn != rinsn)
> +    recog_data.insn = NULL;
> +
>    if (dist_define < 0 || dist_define >= LEA_MAX_STALL)
>      {
>        /* If there is no non AGU operand definition, no AGU
> --- gcc/testsuite/gcc.target/i386/pr93611.c.jj  2020-02-06 12:24:28.005976435 +0100
> +++ gcc/testsuite/gcc.target/i386/pr93611.c     2020-02-06 12:24:17.685131826 +0100
> @@ -0,0 +1,5 @@
> +/* PR target/93611 */
> +/* { dg-do compile } */
> +/* { dg-options "-fira-algorithm=priority -O3 -mtune=bonnell" } */
> +
> +#include "../../gcc.dg/vect/pr58508.c"
>
>         Jakub
>
diff mbox series

Patch

--- gcc/config/i386/i386.c.jj	2020-01-28 08:45:56.781090684 +0100
+++ gcc/config/i386/i386.c	2020-02-06 16:29:35.548663197 +0100
@@ -14459,9 +14459,18 @@  ix86_lea_outperforms (rtx_insn *insn, un
       return true;
     }
 
+  rtx_insn *rinsn = recog_data.insn;
+
   dist_define = distance_non_agu_define (regno1, regno2, insn);
   dist_use = distance_agu_use (regno0, insn);
 
+  /* distance_non_agu_define can call extract_insn_cached.  If this function
+     is called from define_split conditions, that can break insn splitting,
+     because split_insns works by clearing recog_data.insn and then modifying
+     recog_data.operand array and match the various split conditions.  */
+  if (recog_data.insn != rinsn)
+    recog_data.insn = NULL;
+
   if (dist_define < 0 || dist_define >= LEA_MAX_STALL)
     {
       /* If there is no non AGU operand definition, no AGU
--- gcc/testsuite/gcc.target/i386/pr93611.c.jj	2020-02-06 12:24:28.005976435 +0100
+++ gcc/testsuite/gcc.target/i386/pr93611.c	2020-02-06 12:24:17.685131826 +0100
@@ -0,0 +1,5 @@ 
+/* PR target/93611 */
+/* { dg-do compile } */
+/* { dg-options "-fira-algorithm=priority -O3 -mtune=bonnell" } */
+
+#include "../../gcc.dg/vect/pr58508.c"