Re: PATCH: Check all insns in fallthru to see if label is mentioned

Re: PATCH: Check all insns in fallthru to see if label is mentioned

[Richard Kenner]

    > FAIL: gcc.c-torture/execute/strct-pack-1.c execution,  -O2
    > FAIL: gcc.c-torture/execute/strct-pack-1.c execution,  -Os

    It is my guess that these failures are due to the "enhanced structure
    offset tracking" patch.

Reworking that code is pretty high on my list, so I'll take responsibility
for this one.  I hope to get to it by Tuesday.

Re: PATCH: Check all insns in fallthru to see if label is mentioned

[John David Anglin]

>     > FAIL: gcc.c-torture/execute/strct-pack-1.c execution,  -O2
>     > FAIL: gcc.c-torture/execute/strct-pack-1.c execution,  -Os
> 
>     It is my guess that these failures are due to the "enhanced structure
>     offset tracking" patch.
> 
> Reworking that code is pretty high on my list, so I'll take responsibility
> for this one.  I hope to get to it by Tuesday.

In looking at the rtl, I also noticed virtual-stack-vars are being referenced
before they are initialized.  For example, I see the following rtl in main:

(insn 20 18 21 (set (reg:SI 98)
        (mem/s:SI (plus:SI (reg/f:SI 90 virtual-stack-vars)
	        (const_int 8 [0x8])) [5 trial.d+6 S4 A64])) -1 (nil)
    (nil))

(insn 21 20 23 (set (reg:SI 99)
        (and:SI (reg:SI 98)
	    (const_int 65535 [0xffff]))) -1 (nil)
    (nil))

(insn 23 21 25 (set (mem/s:SI (plus:SI (reg/f:SI 90 virtual-stack-vars)
		(const_int 8 [0x8])) [5 trial.d+6 S4 A64])
	(reg:SI 99)) -1 (nil)
    (nil))

What's the point of this?

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

Re: PATCH: Check all insns in fallthru to see if label is mentioned

[Richard Kenner]

    In looking at the rtl, I also noticed virtual-stack-vars are being
    referenced before they are initialized.  For example, I see the
    following rtl in main:

This is to preserve the original value of the other bits in the byte
containing the field.  Yes, they are undefined here, but the bitfield
code can't know that.

Re: PATCH: Check all insns in fallthru to see if label is mentioned

[John David Anglin]

> 2001-10-01  John David Anglin  <dave@hiauly1.hia.nrc.ca>
> 
> 	* cfgcleanup.c (label_mentioned_p): New function.
> 	(try_optimize_cfg): Use it.

In the continuing saga of trying to get the vax bootstrap to work on
the main, I found that the above patch exposes what appears to be a
bug in rtx_equal_p.  It currently may call strcmp with one or both
arguments being null pointers.  This happens with "code_label" rtx's.

The strcmp function on i686-pc-linux-gnu apparently is not prepared to
handle null pointers and generates a segmentation fault.  My original
testing of the patch was on hppa1.1-hp-hpux10.20.  Its strcmp treats
null pointers the same as pointers to empty strings.

I have updated the patch to fix this problem.  Bootstrapped and checked
on i686-pc-linux-gnu.

OK?

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

2001-10-03  John David Anglin  <dave@hiauly1.hia.nrc.ca>

	* cfgcleanup.c (label_mentioned_p): New function.
	(try_optimize_cfg): Use it.

	* rtl.c (rtx_equal_p): Check for null pointers when comparing rtx
	strings.

--- cfgcleanup.c.orig	Fri Sep 21 18:29:23 2001
+++ cfgcleanup.c	Fri Sep 28 16:09:38 2001
@@ -52,6 +52,7 @@
 						 rtx *, rtx *));
 
 static bool delete_unreachable_blocks	PARAMS ((void));
+static bool label_mentioned_p		PARAMS ((rtx, rtx));
 static bool tail_recursion_label_p	PARAMS ((rtx));
 static void merge_blocks_move_predecessor_nojumps PARAMS ((basic_block,
 							  basic_block));
@@ -248,6 +249,22 @@
   return changed;
 }
 \f
+/* Return true if LABEL is mentioned in any insn from INSN up to
+   but not including the label.  */
+
+static bool
+label_mentioned_p (label, insn)
+     rtx label, insn;
+{
+  rtx x;
+
+  for (x = insn; x != label; x = NEXT_INSN (x))
+    if (reg_mentioned_p (label, x))
+      return true;
+
+  return false;
+}
+
 /* Return true if LABEL is used for tail recursion.  */
 
 static bool
@@ -1096,7 +1113,7 @@
 	      /* If previous block ends with condjump jumping to next BB,
 	         we can't delete the label.  */
 	      && (b->pred->src == ENTRY_BLOCK_PTR
-		  || !reg_mentioned_p (b->head, b->pred->src->end)))
+		  || !label_mentioned_p (b->head, b->pred->src->end)))
 	    {
 	      rtx label = b->head;
 	      b->head = NEXT_INSN (b->head);
--- rtl.c.orig	Wed Aug 22 11:33:57 2001
+++ rtl.c	Wed Oct  3 11:34:19 2001
@@ -650,7 +650,9 @@
 
 	case 'S':
 	case 's':
-	  if (strcmp (XSTR (x, i), XSTR (y, i)))
+	  if ((XSTR (x, i) || XSTR (y, i))
+	      && (! XSTR (x, i) || ! XSTR (y, i)
+		  || strcmp (XSTR (x, i), XSTR (y, i))))
 	    return 0;
 	  break;
 

Re: PATCH: Check all insns in fallthru to see if label is mentioned

[Richard Henderson]

On Wed, Oct 03, 2001 at 03:08:12PM -0400, John David Anglin wrote:
> 	* cfgcleanup.c (label_mentioned_p): New function.
> 	(try_optimize_cfg): Use it.

Rather than search, this check ought to be

              && (!(mode & CLEANUP_PRE_SIBCALL)
                  || !tail_recursion_label_p (b->head))
-             /* If previous block ends with condjump jumping to next BB,
-                we can't delete the label.  */
-             && (b->pred->src == ENTRY_BLOCK_PTR
-                 || !reg_mentioned_p (b->head, b->pred->src->end)))
+	      /* If the previous block ends with a branch to this block,
+		 we can't delete the label.  Normally this is a condjump
+		 that is yet to be simplified, but if CASE_DROPS_THRU,
+		 this can be a tablejump with some element going to the
+		 same place as the default (fallthru).  */
+	      && (b->pred->src == ENTRY_BLOCK_PTR
+		  || GET_CODE (b->pred->src->end) != JUMP_INSN
+		  || ! label_is_jump_target_p (b->head, b->pred->src->end)))


/* Return true if LABEL is a target of JUMP_INSN.  This applies only
   to non-complex jumps.  That is, direct unconditional, conditional,
   and tablejumps, but not computed jumps or returns.  It also does
   not apply to the fallthru case of a conditional jump.  */

bool
label_is_jump_target_p (label, jump_insn)
     rtx label, jump_insn;
{
  rtx tmp = JUMP_LABEL (jump_insn);

  if (label == tmp)
    return true;

  if (tmp != NULL_RTX
      && (tmp = NEXT_INSN (tmp)) != NULL_RTX
      && GET_CODE (tmp) == JUMP_INSN
      && (tmp = PATTERN (tmp),
	  GET_CODE (tmp) == ADDR_VEC
	  || GET_CODE (tmp) == ADDR_DIFF_VEC))
    {
      rtvec vec = XVEC (tmp, GET_CODE (tmp) == ADDR_DIFF_VEC);
      int i, veclen = GET_NUM_ELEM (vec);

      for (i = 0; i < veclen; ++i)
	if (XEXP (RTVEC_ELT (vec, i), 0) == label)
	  return true;
    }

  return false;
}

> 	* rtl.c (rtx_equal_p): Check for null pointers when comparing rtx
> 	strings.

The cfgcleanup thing aside, this is ok.


r~

Re: PATCH: Check all insns in fallthru to see if label is mentioned

[John David Anglin]

> > On Wed, Oct 03, 2001 at 03:08:12PM -0400, John David Anglin wrote:
> > > 	* cfgcleanup.c (label_mentioned_p): New function.
> > > 	(try_optimize_cfg): Use it.
> > 
> 
> Rather than search, this check ought to be

I reworked your comments into a patch for cfgcleanup.c.  The only change
is the addition of `static' to the declaration of label_is_jump_target_p.
I have tested with a complete bootstrap and check under hppa2.0w-hp-hpux11.11.
There might be two regressions:

FAIL: gcc.c-torture/execute/strct-pack-1.c execution,  -O2
FAIL: gcc.c-torture/execute/strct-pack-1.c execution,  -Os

These started failing in execution at some point after Dec. 1.  It will
be a few days before I can examine these in more detail.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

2001-12-08  Richard Henderson  <rth@redhat.com>

	* cfgcleanup.c (label_is_jump_target_p): New function.
	(try_optimize_cfg): Use label_is_jump_target_p to check if label is
	target of a JUMP_INSN from the preceding block.

--- cfgcleanup.c.orig	Thu Nov 15 14:55:33 2001
+++ cfgcleanup.c	Thu Dec  6 11:06:42 2001
@@ -69,6 +69,7 @@
 						 rtx *, rtx *));
 
 static bool delete_unreachable_blocks	PARAMS ((void));
+static bool label_is_jump_target_p	PARAMS ((rtx, rtx));
 static bool tail_recursion_label_p	PARAMS ((rtx));
 static void merge_blocks_move_predecessor_nojumps PARAMS ((basic_block,
 							  basic_block));
@@ -291,6 +292,38 @@
   return changed;
 }
 \f
+/* Return true if LABEL is a target of JUMP_INSN.  This applies only
+   to non-complex jumps.  That is, direct unconditional, conditional,
+   and tablejumps, but not computed jumps or returns.  It also does
+   not apply to the fallthru case of a conditional jump.  */
+
+static bool
+label_is_jump_target_p (label, jump_insn)
+     rtx label, jump_insn;
+{
+  rtx tmp = JUMP_LABEL (jump_insn);
+
+  if (label == tmp)
+    return true;
+
+  if (tmp != NULL_RTX
+      && (tmp = NEXT_INSN (tmp)) != NULL_RTX
+      && GET_CODE (tmp) == JUMP_INSN
+      && (tmp = PATTERN (tmp),
+	  GET_CODE (tmp) == ADDR_VEC
+	  || GET_CODE (tmp) == ADDR_DIFF_VEC))
+    {
+      rtvec vec = XVEC (tmp, GET_CODE (tmp) == ADDR_DIFF_VEC);
+      int i, veclen = GET_NUM_ELEM (vec);
+
+      for (i = 0; i < veclen; ++i)
+	if (XEXP (RTVEC_ELT (vec, i), 0) == label)
+	  return true;
+    }
+
+  return false;
+}
+
 /* Return true if LABEL is used for tail recursion.  */
 
 static bool
@@ -1162,10 +1195,14 @@
 	      && GET_CODE (b->head) == CODE_LABEL
 	      && (!(mode & CLEANUP_PRE_SIBCALL)
 		  || !tail_recursion_label_p (b->head))
-	      /* If previous block ends with condjump jumping to next BB,
-	         we can't delete the label.  */
+	      /* If the previous block ends with a branch to this block,
+		 we can't delete the label.  Normally this is a condjump
+		 that is yet to be simplified, but if CASE_DROPS_THRU,
+		 this can be a tablejump with some element going to the
+		 same place as the default (fallthru).  */
 	      && (b->pred->src == ENTRY_BLOCK_PTR
-		  || !reg_mentioned_p (b->head, b->pred->src->end)))
+		  || GET_CODE (b->pred->src->end) != JUMP_INSN
+		  || ! label_is_jump_target_p (b->head, b->pred->src->end)))
 	    {
 	      rtx label = b->head;
 	      b->head = NEXT_INSN (b->head);

Re: PATCH: Check all insns in fallthru to see if label is mentioned

[Richard Henderson]

On Sat, Dec 08, 2001 at 12:22:35PM -0500, John David Anglin wrote:
> 	* cfgcleanup.c (label_is_jump_target_p): New function.
> 	(try_optimize_cfg): Use label_is_jump_target_p to check if label is
> 	target of a JUMP_INSN from the preceding block.

Ok.


r~

Re: PATCH: Check all insns in fallthru to see if label is mentioned

[John David Anglin]

> There might be two regressions:
> 
> FAIL: gcc.c-torture/execute/strct-pack-1.c execution,  -O2
> FAIL: gcc.c-torture/execute/strct-pack-1.c execution,  -Os
> 
> These started failing in execution at some point after Dec. 1.  It will
> be a few days before I can examine these in more detail.

It is my guess that these failures are due to the "enhanced structure
offset tracking" patch.

In the -O2 assembler output, we have

        ldi 1,%r20
        .stabn 68,0,20,L$M7-main
	L$M7:
	ldw -120(%r30),%r21
	<manipulation of r21 deleted>
        sth %r20,-120(%r30)
	.stabn 68,0,20,L$M11-main
	L$M11:
	stw %r22,-112(%r30)
	stw %r21,-120(%r30)

The halfword store of r20 gets moved downward, after the load of r21.
This occurs in the ce2 pass.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

PATCH: Check all insns in fallthru to see if label is mentioned

[John David Anglin]

On the vax, a label in an addr_diff_vec can be inadvertantly deleted by
try_optimize_cfg.  Currently, try_optimize_cfg just checks the insn at
the end of the previous block for a condjump which mentions the label.
This patch extends the check to look at all insns from the end of the
previous block up to but not including the label.

Bootstrap checked with no regressions on hppa1.1-hp-hpux10.20.  On the
vax, it has been confirmed that it resolves the problem in question.
Unfortunately, there are still more bugs to stomp.

OK?

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

2001-10-01  John David Anglin  <dave@hiauly1.hia.nrc.ca>

	* cfgcleanup.c (label_mentioned_p): New function.
	(try_optimize_cfg): Use it.

--- cfgcleanup.c.orig	Fri Sep 21 18:29:23 2001
+++ cfgcleanup.c	Fri Sep 28 16:09:38 2001
@@ -52,6 +52,7 @@
 						 rtx *, rtx *));
 
 static bool delete_unreachable_blocks	PARAMS ((void));
+static bool label_mentioned_p		PARAMS ((rtx, rtx));
 static bool tail_recursion_label_p	PARAMS ((rtx));
 static void merge_blocks_move_predecessor_nojumps PARAMS ((basic_block,
 							  basic_block));
@@ -248,6 +249,22 @@
   return changed;
 }
 \f
+/* Return true if LABEL is mentioned in any insn from INSN up to
+   but not including the label.  */
+
+static bool
+label_mentioned_p (label, insn)
+     rtx label, insn;
+{
+  rtx x;
+
+  for (x = insn; x != label; x = NEXT_INSN (x))
+    if (reg_mentioned_p (label, x))
+      return true;
+
+  return false;
+}
+
 /* Return true if LABEL is used for tail recursion.  */
 
 static bool
@@ -1096,7 +1113,7 @@
 	      /* If previous block ends with condjump jumping to next BB,
 	         we can't delete the label.  */
 	      && (b->pred->src == ENTRY_BLOCK_PTR
-		  || !reg_mentioned_p (b->head, b->pred->src->end)))
+		  || !label_mentioned_p (b->head, b->pred->src->end)))
 	    {
 	      rtx label = b->head;
 	      b->head = NEXT_INSN (b->head);