> On Tue, Nov 12, 2019 at 04:32:36PM +0100, Ilya Leoshkevich wrote:
>>> Am 12.11.2019 um 15:32 schrieb Segher Boessenkool
>>> <seg...@kernel.crashing.org>:
>>> On Tue, Nov 12, 2019 at 03:11:05PM +0100, Ilya Leoshkevich wrote:
>>>> unsigned int
>>>> pass_jump_after_combine::execute (function *)
>>>> {
>>>> + /* Jump threading does not keep dominators up-to-date. */
>>>> + free_dominance_info (CDI_DOMINATORS);
>>>> + free_dominance_info (CDI_POST_DOMINATORS);
>>>> cleanup_cfg (flag_thread_jumps ? CLEANUP_THREADING : 0);
>>>> return 0;
>>>> }
>>>
>>> Why do you always free it, if if only gets invalidated if flag_thread_jumps?
>>>
>>> It may be a good idea to throw away the dom info anyway, but the comment
>>> seems off then?
>>
>> Hmm, come to think of it, it would make sense to make flag_thread_jumps
>> a gate for this pass, and then run free_dominance_info (CDI_DOMINATORS)
>> and cleanup_cfg (CLEANUP_THREADING) unconditionally. What do you think?
>
> But we want cleanup_cfg (0) if the flag is not set, no? Maybe
> something like
When I was adding this pass, I didn't really want cleanup_cfg (0) - I
don't think this achieves anything useful at this point. It's just that
I copied the structure of the existing code without thinking too much
about it. But now that you brought it up...
>
> unsigned int
> pass_jump_after_combine::execute (function *)
> {
> if (flag_thread_jumps)
> {
> /* Jump threading does not keep dominators up-to-date. */
> free_dominance_info (CDI_DOMINATORS);
> cleanup_cfg (CLEANUP_THREADING);
> }
> else
> cleanup_cfg (0);
>
> return 0;
> }
>
> But OTOH it may well be the case that other things in cleanup_cfg make
> the known dominance info invalid as well, in which case just the comment
> is a bit misleading. Sounds likely to me :-)
Yeah, that's what I worry about as well. In particular, this block in
try_optimize_cfg:
/* Try to change a conditional branch to a return to the
respective conditional return. */
if (EDGE_COUNT (b->succs) == 2
&& any_condjump_p (BB_END (b))
&& bb_is_just_return (BRANCH_EDGE (b)->dest, &ret, &use))
{
if (redirect_jump (as_a <rtx_jump_insn *> (BB_END (b)),
PATTERN (ret), 0))
{
if (use)
emit_insn_before (copy_insn (PATTERN (use)),
BB_END (b));
if (dump_file)
fprintf (dump_file, "Changed conditional jump %d->%d "
"to conditional return.\n",
b->index, BRANCH_EDGE (b)->dest->index);
redirect_edge_succ (BRANCH_EDGE (b),
EXIT_BLOCK_PTR_FOR_FN (cfun));
BRANCH_EDGE (b)->flags &= ~EDGE_CROSSING;
changed_here = true;
}
}
runs regardless of cleanup mode, and it makes use of redirect_edge_succ,
which does not update dominators.
Best regards,
Ilya
