memory explosion in ifcvt.c

memory explosion in ifcvt.c

[Richard Earnshaw]

Steven,

Your patch to ifcvt.c is causing an infinite loop that eventually runs
out of memory when compiling for arm-eabi.

My guess is that this is likely to be related to the following warning
(you did do a full bootstrap test of this, didn't you -- I would have
expected that to catch a bug like this :-)

.../gcc/ifcvt.c:3239: warning: control reaches end of non-void function

2007-03-26  Steven Bosscher  <steven@gcc.gnu.org>

        * ifcvt.c (noce_try_store_flag_constants): Don't check
        no_new_pseudos here.
	...

Re: memory explosion in ifcvt.c

[Steven Bosscher]

On 3/27/07, Richard Earnshaw <rearnsha@arm.com> wrote:
> Steven,
>
> Your patch to ifcvt.c is causing an infinite loop that eventually runs
> out of memory when compiling for arm-eabi.
>
> My guess is that this is likely to be related to the following warning
> (you did do a full bootstrap test of this, didn't you --

Are you suggesting I did not?  I think I mentioned bootstrapping and
testing on i686 and on x86_64. And when I look at gcc-testresults, I
see lots of other people who have suffessfully bootstrapped and tested
with the patch.

> I would have
> expected that to catch a bug like this :-)

Apparently not. Perhaps you could try to figure out why.

Gr.
Steven

Re: memory explosion in ifcvt.c

[Steven Bosscher]

On 3/27/07, Richard Earnshaw <rearnsha@arm.com> wrote:
> .../gcc/ifcvt.c:3239: warning: control reaches end of non-void function


2007-03-27  Steven Bosscher  <steven@gcc.gnu.org>

        * ifcvt.c (cond_exec_find_if_block): Return FALSE if no
transformations are
        applied successfully.


Index: ifcvt.c
===================================================================
--- ifcvt.c     (revision 123223)
+++ ifcvt.c     (working copy)
@@ -3236,6 +3236,8 @@ cond_exec_find_if_block (struct ce_if_bl
       if (cond_exec_process_if_block (ce_info, FALSE))
        return TRUE;
     }
+
+  return FALSE;
 }

 /* Convert a branch over a trap, or a branch

Re: memory explosion in ifcvt.c

[Ian Lance Taylor]

"Steven Bosscher" <stevenb.gcc@gmail.com> writes:

> On 3/27/07, Richard Earnshaw <rearnsha@arm.com> wrote:
> > .../gcc/ifcvt.c:3239: warning: control reaches end of non-void function
> 
> 
> 2007-03-27  Steven Bosscher  <steven@gcc.gnu.org>
> 
>         * ifcvt.c (cond_exec_find_if_block): Return FALSE if no
> transformations are
>         applied successfully.

Counts as obvious.

Ian

Re: memory explosion in ifcvt.c

[Steven Bosscher]

On 27 Mar 2007 15:24:07 -0700, Ian Lance Taylor <iant@google.com> wrote:
> "Steven Bosscher" <stevenb.gcc@gmail.com> writes:
>
> > On 3/27/07, Richard Earnshaw <rearnsha@arm.com> wrote:
> > > .../gcc/ifcvt.c:3239: warning: control reaches end of non-void function
> >
> >
> > 2007-03-27  Steven Bosscher  <steven@gcc.gnu.org>
> >
> >         * ifcvt.c (cond_exec_find_if_block): Return FALSE if no
> > transformations are
> >         applied successfully.
>
> Counts as obvious.

Commited after bootstrapping&testing on ia64.

Gr.
Steven

Re: memory explosion in ifcvt.c

[Steven Bosscher]

[-- Attachment #1: Type: text/plain, Size: 677 bytes --]

On 3/28/07, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> Commited after bootstrapping&testing on ia64.

Life wouldn't be as much fun if I didn't find regressions every time I
test on this patform. I can even blame myself for this regression,
gcc.dg/ifcvt-fabs.c.

There is another noce_get_condition function, and it wasn't prepared
to handle IF blocks where the ELSE block is the fallthru block. Which
happens to be the exceptional case, actually, and I had somehow mixed
that up in my mind and thought that the exception was to have the THEN
block as the falltrhu block. So I fixed my comments while there.

I'll bootstrap&test this again before the weekend.

Gr.
Steven

[-- Attachment #2: ifcvt-fabs_c-fix.diff --]
[-- Type: text/x-patch, Size: 2678 bytes --]

	* ifcvt.c (struct noce_if_info): Add then_else_reversed field.
	(noce_get_alt_condition): Look at it to determine whether to
	reverse the condition or not.
	(noce_get_condition): Substitute the truth for lies.
	(noce_find_if_block): Set the then_else_reversed field.

Index: ifcvt.c
===================================================================
--- ifcvt.c	(revision 123289)
+++ ifcvt.c	(working copy)
@@ -599,6 +599,12 @@ struct noce_if_info
   /* The basic blocks that make up the IF-THEN-{ELSE-,}JOIN block.  */
   basic_block test_bb, then_bb, else_bb, join_bb;
 
+  /* True it this if block is not canonical.  In the canonical form of
+     if blocks, the THEN_BB is the block reached via the fallthru edge
+     from TEST_BB.  For the noce transformations, we allow the symmetric
+     form as well.  */
+  bool then_else_reversed;
+
   /* The jump that ends TEST_BB.  */
   rtx jump;
 
@@ -1503,6 +1509,8 @@ noce_get_alt_condition (struct noce_if_i
   reverse
     = GET_CODE (XEXP (SET_SRC (set), 2)) == LABEL_REF
       && XEXP (XEXP (SET_SRC (set), 2), 0) == JUMP_LABEL (if_info->jump);
+  if (if_info->then_else_reversed)
+    reverse = !reverse;
 
   /* If we're looking for a constant, try to make the conditional
      have that constant in it.  There are two reasons why it may
@@ -2017,8 +2025,8 @@ noce_try_bitop (struct noce_if_info *if_
 /* Similar to get_condition, only the resulting condition must be
    valid at JUMP, instead of at EARLIEST.
 
-   If THEN_ELSE_REVERSED is true, the fallthrough goes to the THEN
-   block of the caller, and we have to reverse the condition.  */
+   If THEN_ELSE_REVERSED is true, the fallthrough does not go to the
+   THEN block of the caller, and we have to reverse the condition.  */
 
 static rtx
 noce_get_condition (rtx jump, rtx *earliest, bool then_else_reversed)
@@ -2036,8 +2044,9 @@ noce_get_condition (rtx jump, rtx *earli
   reverse = (GET_CODE (XEXP (SET_SRC (set), 2)) == LABEL_REF
 	     && XEXP (XEXP (SET_SRC (set), 2), 0) == JUMP_LABEL (jump));
 
-  /* We may have to reverse because the caller's if block is not canonical
-     (i.e. the ELSE block isn't the fallthrough block for the TEST block).  */
+  /* We may have to reverse because the caller's if block is not canonical,
+     i.e. the THEN block isn't the fallthrough block for the TEST block
+     (see find_if_header).  */
   if (then_else_reversed)
     reverse = !reverse;
 
@@ -2733,6 +2742,7 @@ noce_find_if_block (basic_block test_bb,
   if_info.then_bb = then_bb;
   if_info.else_bb = else_bb;
   if_info.join_bb = join_bb;
+  if_info.then_else_reversed = then_else_reversed;
   if_info.cond = cond;
   if_info.jump = jump;
 

Re: memory explosion in ifcvt.c

[Richard Henderson]

On Thu, Mar 29, 2007 at 12:23:11AM +0200, Steven Bosscher wrote:
> 	* ifcvt.c (struct noce_if_info): Add then_else_reversed field.
> 	(noce_get_alt_condition): Look at it to determine whether to
> 	reverse the condition or not.
> 	(noce_get_condition): Substitute the truth for lies.
> 	(noce_find_if_block): Set the then_else_reversed field.

Ok, except,

> +  /* True it this if block is not canonical.  In the canonical form of
            ^if

> +     if blocks, the THEN_BB is the block reached via the fallthru edge
> +     from TEST_BB.  For the noce transformations, we allow the symmetric
> +     form as well.  */
> +  bool then_else_reversed;

And please place this field at the end of the structure.
I know there's only one non-pointer field now, but that
might change.


r~