Re: [PATCH] Free dominance info at the beginning of pass_jump_after_combine

[Ilya Leoshkevich <iii at linux dot ibm dot com>]

> On Tue, Nov 12, 2019 at 04:32:36PM +0100, Ilya Leoshkevich wrote:
>>> Am 12.11.2019 um 15:32 schrieb Segher Boessenkool <segher@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