Question about dead_or_predicable

Question about dead_or_predicable

[Steven Bosscher]

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.

I am guessing this code was required when ifcvt.c was contributed (the
file appears in r33547). The code has, at this point, proven that
MERGE_BB is "dead or predicable" and it now tries to rewire the insns
stream and the CFG to apply the transformation. Perhaps historically
the changes to the jump from old_dest to new_dest had to be in the
same change group as the changes to predicate the insns.

But now there is OTHER_BB, which is not documented, and for which the
changes to the jumps are done *after* the change group for everything
else has been applied.  The OTHER_BB is a bit newer (but only a little
bit) than the first revision in SVN of ifcvt.c. These changes are
applied directly, i.e. not in the change group, and it isn't even
verified that the changes are applied successfully?!

What I would like to do, is to apply the change group *before*
changing the jumps. That way, I can move the apply_change_group() call
into the "if (HAVE_conditional_execution)" block higher up in
dead_or_predicable, and try the non-conditional execution case if
predicating the insns fails. When neither succeeds, we're done.  When
one or the other succeeds, we change the jumps and rewire the CFG.

But since in one case (from merge_bb to new_dest) the changes are
applied as part of a change group, and in the other case (other_bb to
new_dest) the changes are not, I'm confused. 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?

Hope the question makes enough sense for someone to help me out here :-)

Ciao!
Steven

Re: Question about dead_or_predicable

[Dave Korn]

Steven Bosscher wrote:

> 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.

  dead_or_predicable is called from find_if_case_[12] which is called from
find_if_header which is called from if_convert within a FOR_EACH_BB loop; is
this perhaps another example of "don't mess with something while you're
iterating over it or your iterator might break"?

    cheers,
      DaveK

Re: Question about dead_or_predicable

[Steven Bosscher]

On Wed, Jun 24, 2009 at 1:49 AM, Dave
Korn<dave.korn.cygwin@googlemail.com> wrote:
> Steven Bosscher wrote:
>
>> 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.
>
>  dead_or_predicable is called from find_if_case_[12] which is called from
> find_if_header which is called from if_convert within a FOR_EACH_BB loop; is
> this perhaps another example of "don't mess with something while you're
> iterating over it or your iterator might break"?

I don't think so. I know the rest of ifcvt.c pretty well. The code
expects blocks to be merged, moved, etc. when it has identified an
if-block.

Ciao!
Steven

Re: Question about dead_or_predicable

[Richard Henderson]

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~

Re: Question about dead_or_predicable

[Steven Bosscher]

On Wed, Jun 24, 2009 at 5:35 PM, Richard Henderson<rth@redhat.com> wrote:

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

Heh, well, all those functions in jump.c also really confuse me --
probably because I've never really understood how all the functions to
change RTL work.  But OK, I understand what is going on now, thanks.


> 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.

Right.  I think I'll do it with a verify_changes() check at the end of
the CE path, that should result in a smaller patch.

Thanks for your help,

Ciao!
Steven