Re: Flow crash for conditional branches against a constant

Re: Flow crash for conditional branches against a constant

[Daniel Jacobowitz]

On Fri, Jun 11, 2004 at 01:05:34PM -0400, Daniel Jacobowitz wrote:
> Rather than adding it I'm testing removal of the invariant.  I don't
> see anything besides the boolean arithmetic functions which relies on
> the exact form of the condition... yet.

I still don't see any problems.  The attached patch causes no
differences in the generated libstdc++ on arm-elf (both -marm and
-mthumb), and no testsuite changes for C, C++, or libstdc++-v3 (again
-marm and -mthumb).

[Well, no real testsuite changes.  Four libstdc++ tests failed, but
pass when rerunning them by hand; the error is symptomatic of a missing
input file.  Two g++ tests pass, where they used to fail to link
referencing a symbol in libstdc++, but libstdc++ is exactly the same. 
I think I botched something running the tests.]

I imagine there's some case where this permits slightly better code on
Thumb.  It also allows me to use cond_exec patterns which compare
against non-zero constants.

OK?

-- 
Daniel Jacobowitz

2004-06-12  Daniel Jacobowitz  <dan@debian.org>

	* flow.c (init_propagate_block_info): Reallow more complex
	conditions.
	(ior_reg_cond, not_reg_cond, and_reg_cond, elim_reg_cond): Handle
	more complex conditions.

Index: flow.c
===================================================================
RCS file: /big/fsf/rsync/gcc-cvs/gcc/gcc/flow.c,v
retrieving revision 1.588
diff -u -p -r1.588 flow.c
--- a/flow.c	15 May 2004 09:39:29 -0000	1.588
+++ b/flow.c	11 Jun 2004 19:35:32 -0000
@@ -1865,51 +1865,43 @@ init_propagate_block_info (basic_block b
 	  rtx set_src = SET_SRC (pc_set (BB_END (bb)));
 	  rtx cond_true = XEXP (set_src, 0);
 	  rtx reg = XEXP (cond_true, 0);
+	  rtx cond_false
+	    = gen_rtx_fmt_ee (reverse_condition (GET_CODE (cond_true)),
+			      GET_MODE (cond_true), XEXP (cond_true, 0),
+			      XEXP (cond_true, 1));
 
 	  if (GET_CODE (reg) == SUBREG)
 	    reg = SUBREG_REG (reg);
 
-	  /* We can only track conditional lifetimes if the condition is
-	     in the form of a comparison of a register against zero.  
-	     If the condition is more complex than that, then it is safe
-	     not to record any information.  */
-	  if (GET_CODE (reg) == REG
-	      && XEXP (cond_true, 1) == const0_rtx)
+	  if (GET_CODE (XEXP (set_src, 1)) == PC)
 	    {
-	      rtx cond_false
-		= gen_rtx_fmt_ee (reverse_condition (GET_CODE (cond_true)),
-				  GET_MODE (cond_true), XEXP (cond_true, 0),
-				  XEXP (cond_true, 1));
-	      if (GET_CODE (XEXP (set_src, 1)) == PC)
-		{
-		  rtx t = cond_false;
-		  cond_false = cond_true;
-		  cond_true = t;
-		}
+	      rtx t = cond_false;
+	      cond_false = cond_true;
+	      cond_true = t;
+	    }
 
-	      SET_REGNO_REG_SET (pbi->reg_cond_reg, REGNO (reg));
+	  SET_REGNO_REG_SET (pbi->reg_cond_reg, REGNO (reg));
 
-	      /* For each such register, mark it conditionally dead.  */
-	      EXECUTE_IF_SET_IN_REG_SET
-		(diff, 0, i,
-		 {
-		   struct reg_cond_life_info *rcli;
-		   rtx cond;
-
-		   rcli = xmalloc (sizeof (*rcli));
-
-		   if (REGNO_REG_SET_P (bb_true->global_live_at_start, i))
-		     cond = cond_false;
-		   else
-		     cond = cond_true;
-		   rcli->condition = cond;
-		   rcli->stores = const0_rtx;
-		   rcli->orig_condition = cond;
-
-		   splay_tree_insert (pbi->reg_cond_dead, i,
-				      (splay_tree_value) rcli);
-		 });
-	    }
+	  /* For each such register, mark it conditionally dead.  */
+	  EXECUTE_IF_SET_IN_REG_SET
+	    (diff, 0, i,
+	    {
+	      struct reg_cond_life_info *rcli;
+	      rtx cond;
+
+	      rcli = xmalloc (sizeof (*rcli));
+
+	      if (REGNO_REG_SET_P (bb_true->global_live_at_start, i))
+		cond = cond_false;
+	      else
+		cond = cond_true;
+	      rcli->condition = cond;
+	      rcli->stores = const0_rtx;
+	      rcli->orig_condition = cond;
+
+	      splay_tree_insert (pbi->reg_cond_dead, i,
+				 (splay_tree_value) rcli);
+	    });
 	}
 
       FREE_REG_SET (diff);
@@ -2994,11 +2986,19 @@ ior_reg_cond (rtx old, rtx x, int add)
     {
       if (COMPARISON_P (x)
 	  && REVERSE_CONDEXEC_PREDICATES_P (GET_CODE (x), GET_CODE (old))
-	  && REGNO (XEXP (x, 0)) == REGNO (XEXP (old, 0)))
+	  && rtx_equal_p (XEXP (x, 0), XEXP (old, 0))
+	  && rtx_equal_p (XEXP (x, 1), XEXP (old, 1)))
 	return const1_rtx;
-      if (GET_CODE (x) == GET_CODE (old)
-	  && REGNO (XEXP (x, 0)) == REGNO (XEXP (old, 0)))
+
+      if (GET_CODE (x) == GET_CODE (old) && rtx_equal_p (x, old))
 	return old;
+
+      if (COMPARISON_P (x)
+	  && swap_condition (GET_CODE (x)) == GET_CODE (old)
+	  && rtx_equal_p (XEXP (x, 0), XEXP (old, 1))
+	  && rtx_equal_p (XEXP (x, 1), XEXP (old, 0)))
+	return old;
+
       if (! add)
 	return NULL;
       return gen_rtx_IOR (0, old, x);
@@ -3085,15 +3085,9 @@ not_reg_cond (rtx x)
   x_code = GET_CODE (x);
   if (x_code == NOT)
     return XEXP (x, 0);
-  if (COMPARISON_P (x)
-      && GET_CODE (XEXP (x, 0)) == REG)
-    {
-      if (XEXP (x, 1) != const0_rtx)
-	abort ();
-
-      return gen_rtx_fmt_ee (reverse_condition (x_code),
-			     VOIDmode, XEXP (x, 0), const0_rtx);
-    }
+  if (COMPARISON_P (x))
+    return gen_rtx_fmt_ee (reverse_condition (x_code),
+			   VOIDmode, XEXP (x, 0), XEXP (x, 1));
   return gen_rtx_NOT (0, x);
 }
 
@@ -3106,11 +3100,18 @@ and_reg_cond (rtx old, rtx x, int add)
     {
       if (COMPARISON_P (x)
 	  && GET_CODE (x) == reverse_condition (GET_CODE (old))
-	  && REGNO (XEXP (x, 0)) == REGNO (XEXP (old, 0)))
+	  && rtx_equal_p (XEXP (x, 0), XEXP (old, 0))
+	  && rtx_equal_p (XEXP (x, 1), XEXP (old, 1)))
 	return const0_rtx;
-      if (GET_CODE (x) == GET_CODE (old)
-	  && REGNO (XEXP (x, 0)) == REGNO (XEXP (old, 0)))
+
+      if (GET_CODE (x) == GET_CODE (old) && rtx_equal_p (x, old))
 	return old;
+      if (COMPARISON_P (x)
+	  && GET_CODE (x) == swap_condition (GET_CODE (old))
+	  && rtx_equal_p (XEXP (x, 0), XEXP (old, 1))
+	  && rtx_equal_p (XEXP (x, 1), XEXP (old, 0)))
+	return old;
+
       if (! add)
 	return NULL;
       return gen_rtx_AND (0, old, x);
@@ -3197,7 +3198,8 @@ elim_reg_cond (rtx x, unsigned int regno
 
   if (COMPARISON_P (x))
     {
-      if (REGNO (XEXP (x, 0)) == regno)
+      int nregs = HARD_REGNO_NREGS (regno, GET_MODE (XEXP (x, 0)));
+      if (refers_to_regno_p (regno, regno + nregs, x, NULL))
 	return const0_rtx;
       return x;
     }

Re: Flow crash for conditional branches against a constant

[Richard Earnshaw]

On Sat, 2004-06-12 at 21:46, Daniel Jacobowitz wrote:
> On Fri, Jun 11, 2004 at 01:05:34PM -0400, Daniel Jacobowitz wrote:
> > Rather than adding it I'm testing removal of the invariant.  I don't
> > see anything besides the boolean arithmetic functions which relies on
> > the exact form of the condition... yet.
> 
> I still don't see any problems.  The attached patch causes no
> differences in the generated libstdc++ on arm-elf (both -marm and
> -mthumb), and no testsuite changes for C, C++, or libstdc++-v3 (again
> -marm and -mthumb).
> 
> [Well, no real testsuite changes.  Four libstdc++ tests failed, but
> pass when rerunning them by hand; the error is symptomatic of a missing
> input file.  Two g++ tests pass, where they used to fail to link
> referencing a symbol in libstdc++, but libstdc++ is exactly the same. 
> I think I botched something running the tests.]
> 
> I imagine there's some case where this permits slightly better code on
> Thumb.  It also allows me to use cond_exec patterns which compare
> against non-zero constants.
> 
> OK?

No, I don't think so (unless I've misunderstood your patch).

The problem is that we only track the one register in the cond table, by
removing the constraint that the second argument is against 0, you
potentially allow

	if (a < b)
		op1;
	else if (a >= c)
		op2;

to be transformed into 

	if (a < b)
		op1;
	else
		op2;

which it might do because it only takes into account 2 of the 3
constraints required (operand 1 is 'a', comparison is opposite of '<')
and ignores the third (operand 2 is 'b').  This is clearly not the same.

R.

Re: Flow crash for conditional branches against a constant

[Daniel Jacobowitz]

On Mon, Jun 14, 2004 at 10:52:17AM +0100, Richard Earnshaw wrote:
> No, I don't think so (unless I've misunderstood your patch).
> 
> The problem is that we only track the one register in the cond table, by
> removing the constraint that the second argument is against 0, you
> potentially allow
> 
> 	if (a < b)
> 		op1;
> 	else if (a >= c)
> 		op2;
> 
> to be transformed into 
> 
> 	if (a < b)
> 		op1;
> 	else
> 		op2;
> 
> which it might do because it only takes into account 2 of the 3
> constraints required (operand 1 is 'a', comparison is opposite of '<')
> and ignores the third (operand 2 is 'b').  This is clearly not the same.

Could you explain where you think this transformation might happen? 
The only things which examine the form of the comparisons in flow.c are
elim_reg_cond, and_reg_cond, not_reg_cond, and ior_reg_cond; they used
to ignore the second operand, but I changed that.

I guess this is the table you're referring to:
  /* Indexed by register number, holds a reg_cond_life_info for each
     register that is not unconditionally live or dead.  */
  splay_tree reg_cond_dead;

But that is indexed by the register we're tracking, which is not a, b,
or c; it's something set by op1.

-- 
Daniel Jacobowitz