Re: asm goto vs simulate_block

[Daniel Berlin <dberlin@dberlin.org>]

My guess, witjout seeing the testcase.
In ccp_initialize we have:

      for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i))
        {
          gimple stmt = gsi_stmt (i);
          bool is_varying = surely_varying_stmt_p (stmt);

          if (is_varying)
            {
              tree def;
              ssa_op_iter iter;

              /* If the statement will not produce a constant, mark
                 all its outputs VARYING.  */
              FOR_EACH_SSA_TREE_OPERAND (def, stmt, iter, SSA_OP_ALL_DEFS)
                set_value_varying (def);
            }
          prop_set_simulate_again (stmt, !is_varying);

This code looks clearly broken if the statement is control altering
(like your asm), since it will cause us to never simulate it and add
the outgoing edges (since the outgoing edges are only added in
simulate_stmt).

Without hacking through the abstraction (since it's ssa propagate that
adds the outgoing edges on simulation), the only thing i can see to do
would be to change the conditional to if (is_varying &&
!stmt_ends_bb_p (bb)) (or the equivalent) so that it simulates them.
Or add a way to tell the propagator about edges it should add
initially to the control worklist.




On Thu, Aug 27, 2009 at 1:23 PM, Richard Henderson<rth@twiddle.net> wrote:
> The kernel folk here at Red Hat have given me a test case (which I'll be
> happy to forward, along a complete patch vs mainline) which gets miscompiled
> because we never get around to adding all of the appropriate blocks outgoing
> from an asm-goto to the simulation.
>
> I can't figure out why the VARYING that we get in simulate_stmt and
> subsequent calls to add_control_edge are not enough to DTRT.  All I know is
> that the attached patch does in fact work around the problem, changing the
> .028t.ccp1 dump:
>
> ...
>  Lattice value changed to VARYING.  Adding SSA edges to worklist.
> +Adding Destination of edge (13 -> 14) to worklist
> +
> +
> +Simulating block 14
> ...
>
> Can someone give me a good explanation as to why this patch would be needed?
>
>
> r~
>