Re: [patch] for PR 14229

[Zdenek Dvorak <rakdver at atrey dot karlin dot mff dot cuni dot cz>]

Hello,

> > the last statement.  Tidy_fallthru_edge is a no-op in this case, which
> > is OK on other places where it is used, but not here.
> 
> So the problem is that try_simplify_condjump assumes that
> tidy_fallthru_edge will clean up after us, but in the case of a
> !only_jump_p it does not?  The only thing that tidy_fallthru edge might
> do is set an edge to fallthough.  It isn't clear why this matters.
> 
> In fact, looking at rtl_tidy_fallthru_edge, I see that it is already
> confused. There is an !onlyjump_p test at the top, and it returns if
> true.  Then in the middle, it tests onlyjump_p as part of an if, but
> this test is redundant.  It will always succeed, because of the earlier
> test.  Since rtl_tidy_fallthru_edge is already broken, maybe the bug is
> there.
> 
> I was really hoping for an analysis and explanation of the patch here. 
> I believe that we need to understand what the code does before we can
> fix it.  A patch that appears to give correct results is not always a
> correct patch.  I did not get an explanation I could understand here.
> 
> I debugged the problem myself so I could see what was going on.  The
> problem seems to be that the delete_basic_block call deletes the
> following block and the edge that leads to it.  That leaves us with a
> conditional branch with only one outgoing edge, which is not marked as a
> fall-through edge.  Other code interprets this as an error unless there
> is a barrier present, and there is no barrier in this case, there should
> not be one.
> 
> So we do need rtl_tidy_fallthru_edge to mark the edge as a fallthru
> edge, or else we need to disable the entire optimization at the top of
> try_simplify_condjump which is what your patch does.
> 
> It isn't clear why rtl_tidy_fallthru_edge ignores !onlyjump_p branches. 
> One could perhaps fix them by deleting the branch and re-emiting the
> rest of the instructions in the parallel.  The current code which leaves
> them unoptimized seems to unfairly penalize targets with
> decrement_and_branch patterns like HPPA.  But this is an optimization
> issue, and it is reasonable to handle this separately from the
> correctness issue.
> 
> The patch is approved.

actually considering the facts, the following patch might be more
reasonable.  It passes bootstrap & regtesting on i686 (sorry for
claiming reverse in my previous mail, I have missinterpreted results
of the testing; I have rerun the tests successfully now).  I am now
running tests on ia64 and ppc64 (where it should encounter some
!onlyjump jumps), it would be nice if someone could try testing it on
HPPA.

Zdenek

Index: cfgrtl.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cfgrtl.c,v
retrieving revision 1.108
diff -c -3 -p -r1.108 cfgrtl.c
*** cfgrtl.c	2 Feb 2004 00:17:08 -0000	1.108
--- cfgrtl.c	20 Feb 2004 23:44:56 -0000
*************** rtl_tidy_fallthru_edge (edge e)
*** 1130,1140 ****
    rtx q;
    basic_block b = e->src, c = b->next_bb;
  
-   /* If the jump insn has side effects, we can't tidy the edge.  */
-   if (GET_CODE (BB_END (b)) == JUMP_INSN
-       && !onlyjump_p (BB_END (b)))
-     return;
- 
    /* ??? In a late-running flow pass, other folks may have deleted basic
       blocks by nopping out blocks, leaving multiple BARRIERs between here
       and the target label. They ought to be chastized and fixed.
--- 1130,1135 ----