Re: Question about dead_or_predicable

[Richard Henderson <rth at redhat dot com>]

On 06/23/2009 03:27 PM, Steven Bosscher wrote:
> Hi,
>
> I have a question about ifcvt.c:dead_or_predicable.  This function is
> pretty complicated and it's not really clear to me what it is doing.
> But I'll have to understand what is going on because there is a bug in
> this function that I would like to fix (see
> http://gcc.gnu.org/PR40525).
>
> The code I don't understand, is the part where the changes are
> actually applied.  This code starts with the following comment:
>
>   33547        rth
>   33547        rth  no_body:
>   33547        rth   /* We don't want to use normal invert_jump or
> redirect_jump because
>   33547        rth      we don't want to delete_insn called.  Also, we
> want to do our own
>   33547        rth      change group management.  */
>   33547        rth
>
> The comment doesn't explain *why* we don't want delete_insn to be
> called, or why we want to do our own change group management.

Have a quick look at the implementation of invert_jump, and both
questions are quickly answered.

First, invert_jump calls apply_change_group, which we clearly
don't want -- thus the comment about "own change group mgmt".

Second, the third argument to invert_jump is delete_unused,
which (if true) eventually results in a call to delete_related_insns,
which we also don't want.

> Can I assume that the
> changes to the jumps never fail? Or should the changes to redirect
> other_bb to new_dest actually be part of the big change group (or at
> least, should it be somehow verified that these changes are applied
> successfully) and is it a bug in ifcvt.c that these changes are
> applied without checks?

They aren't applied without checks.

      if (reversep
          ? ! invert_jump_1 (jump, new_label)
          : ! redirect_jump_1 (jump, new_label))
        goto cancel;

invert and redirect_jump_1 are returning a success value, and they
do apply their changes to the larger change group, all of which is
verified by the apply_change_group call just a few lines down.

All that said, I think your plan to rearrange things to allow both
CE and NCE paths to be followed is fine.  You'll just need to dup
that jump change into both paths so that the CE path change group
gets its own shot at rejecting the jump change.


r~