Allow USE in PARALLELs in store_data_bypass_p (take 2)

Message ID 20171211115204.GJ2353@tucnak
State New
Headers show
Series
  • Allow USE in PARALLELs in store_data_bypass_p (take 2)
Related show

Commit Message

Jakub Jelinek Dec. 11, 2017, 11:52 a.m.
On Mon, Dec 11, 2017 at 12:09:14PM +0100, Eric Botcazou wrote:
> > Is that long enough to be worth it?  I mean, in all other places (rtlanal.c,

> > recog.c, ...) we use similar code in all spots where it is needed, adding

> > an inline would just mean yet another thing to remember.  Or do you mean

> > CLOBBER_OR_USE_P macro?

> 

> No, the whole function, it seems to duplicate everything in the 2 main arms.


Ah, that makes a lot of sense.  So like this?

2017-12-11  Jakub Jelinek  <jakub@redhat.com>

	* recog.c (store_data_bypass_p_1): New function.
	(store_data_bypass_p): Handle USE in a PARALLEL like CLOBBER.  Use
	store_data_bypass_p_1 to avoid code duplication.  Formatting fixes.



	Jakub

Comments

Eric Botcazou Dec. 11, 2017, 12:26 p.m. | #1
> Ah, that makes a lot of sense.  So like this?

> 

> 2017-12-11  Jakub Jelinek  <jakub@redhat.com>

> 

> 	* recog.c (store_data_bypass_p_1): New function.

> 	(store_data_bypass_p): Handle USE in a PARALLEL like CLOBBER.  Use

> 	store_data_bypass_p_1 to avoid code duplication.  Formatting fixes.


Yes, but I think that you can further simplify the first function:

  rtx out_set = single_set (out_insn);
  if (out_set)
    return !reg_mentioned_p (SET_DEST (out_set), SET_DEST (in_set)));

I also wonder why we have a test on PARALLEL in the first one and an assertion 
on the same PARALLEL in the second one.

No big deal in either case so your call for the definitive version.

-- 
Eric Botcazou
Jakub Jelinek Dec. 11, 2017, 12:28 p.m. | #2
On Mon, Dec 11, 2017 at 01:26:42PM +0100, Eric Botcazou wrote:
> > Ah, that makes a lot of sense.  So like this?

> > 

> > 2017-12-11  Jakub Jelinek  <jakub@redhat.com>

> > 

> > 	* recog.c (store_data_bypass_p_1): New function.

> > 	(store_data_bypass_p): Handle USE in a PARALLEL like CLOBBER.  Use

> > 	store_data_bypass_p_1 to avoid code duplication.  Formatting fixes.

> 

> Yes, but I think that you can further simplify the first function:

> 

>   rtx out_set = single_set (out_insn);

>   if (out_set)

>     return !reg_mentioned_p (SET_DEST (out_set), SET_DEST (in_set)));


Ok.

> I also wonder why we have a test on PARALLEL in the first one and an assertion 

> on the same PARALLEL in the second one.


The old code was inconsistent, had return false; in one case and assert in
the remaining two spots.  If you are not against it, I'd use return false; in both
cases if we want consistency.

> No big deal in either case so your call for the definitive version.


	Jakub
Eric Botcazou Dec. 11, 2017, 9:22 p.m. | #3
> The old code was inconsistent, had return false; in one case and assert in

> the remaining two spots.  If you are not against it, I'd use return false;

> in both cases if we want consistency.


Sure, thanks.

-- 
Eric Botcazou

Patch

--- gcc/recog.c.jj	2017-12-06 16:32:53.605660887 +0100
+++ gcc/recog.c	2017-12-11 12:49:05.350575530 +0100
@@ -3657,93 +3657,70 @@  peephole2_optimize (void)
 
 /* Common predicates for use with define_bypass.  */
 
-/* True if the dependency between OUT_INSN and IN_INSN is on the store
-   data not the address operand(s) of the store.  IN_INSN and OUT_INSN
-   must be either a single_set or a PARALLEL with SETs inside.  */
+/* Helper function for store_data_bypass_p, handle just a single SET
+   IN_SET.  */
 
-int
-store_data_bypass_p (rtx_insn *out_insn, rtx_insn *in_insn)
+static bool
+store_data_bypass_p_1 (rtx_insn *out_insn, rtx in_set)
 {
-  rtx out_set, in_set;
-  rtx out_pat, in_pat;
-  rtx out_exp, in_exp;
-  int i, j;
+  if (!MEM_P (SET_DEST (in_set)))
+    return false;
 
-  in_set = single_set (in_insn);
-  if (in_set)
+  rtx out_set = single_set (out_insn);
+  if (out_set)
     {
-      if (!MEM_P (SET_DEST (in_set)))
+      if (reg_mentioned_p (SET_DEST (out_set), SET_DEST (in_set)))
 	return false;
-
-      out_set = single_set (out_insn);
-      if (out_set)
-        {
-          if (reg_mentioned_p (SET_DEST (out_set), SET_DEST (in_set)))
-            return false;
-        }
-      else
-        {
-          out_pat = PATTERN (out_insn);
-
-	  if (GET_CODE (out_pat) != PARALLEL)
-	    return false;
-
-          for (i = 0; i < XVECLEN (out_pat, 0); i++)
-          {
-            out_exp = XVECEXP (out_pat, 0, i);
-
-            if (GET_CODE (out_exp) == CLOBBER)
-              continue;
-
-            gcc_assert (GET_CODE (out_exp) == SET);
-
-            if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_set)))
-              return false;
-          }
-      }
     }
   else
     {
-      in_pat = PATTERN (in_insn);
-      gcc_assert (GET_CODE (in_pat) == PARALLEL);
+      rtx out_pat = PATTERN (out_insn);
+
+      if (GET_CODE (out_pat) != PARALLEL)
+	return false;
 
-      for (i = 0; i < XVECLEN (in_pat, 0); i++)
+      for (int i = 0; i < XVECLEN (out_pat, 0); i++)
 	{
-	  in_exp = XVECEXP (in_pat, 0, i);
+	  rtx out_exp = XVECEXP (out_pat, 0, i);
 
-	  if (GET_CODE (in_exp) == CLOBBER)
+	  if (GET_CODE (out_exp) == CLOBBER || GET_CODE (out_exp) == USE)
 	    continue;
 
-	  gcc_assert (GET_CODE (in_exp) == SET);
+	  gcc_assert (GET_CODE (out_exp) == SET);
 
-	  if (!MEM_P (SET_DEST (in_exp)))
+	  if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_set)))
 	    return false;
+	}
+    }
+
+  return true;
+}
 
-          out_set = single_set (out_insn);
-          if (out_set)
-            {
-              if (reg_mentioned_p (SET_DEST (out_set), SET_DEST (in_exp)))
-                return false;
-            }
-          else
-            {
-              out_pat = PATTERN (out_insn);
-              gcc_assert (GET_CODE (out_pat) == PARALLEL);
-
-              for (j = 0; j < XVECLEN (out_pat, 0); j++)
-                {
-                  out_exp = XVECEXP (out_pat, 0, j);
-
-                  if (GET_CODE (out_exp) == CLOBBER)
-                    continue;
-
-                  gcc_assert (GET_CODE (out_exp) == SET);
-
-                  if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_exp)))
-                    return false;
-                }
-            }
-        }
+/* True if the dependency between OUT_INSN and IN_INSN is on the store
+   data not the address operand(s) of the store.  IN_INSN and OUT_INSN
+   must be either a single_set or a PARALLEL with SETs inside.  */
+
+int
+store_data_bypass_p (rtx_insn *out_insn, rtx_insn *in_insn)
+{
+  rtx in_set = single_set (in_insn);
+  if (in_set)
+    return store_data_bypass_p_1 (out_insn, in_set);
+
+  rtx in_pat = PATTERN (in_insn);
+  gcc_assert (GET_CODE (in_pat) == PARALLEL);
+
+  for (int i = 0; i < XVECLEN (in_pat, 0); i++)
+    {
+      rtx in_exp = XVECEXP (in_pat, 0, i);
+
+      if (GET_CODE (in_exp) == CLOBBER || GET_CODE (in_exp) == USE)
+	continue;
+
+      gcc_assert (GET_CODE (in_exp) == SET);
+
+      if (!store_data_bypass_p_1 (out_insn, in_exp))
+	return false;
     }
 
   return true;