Re: [PATCH, v3] Fix PR51513, switch statement with default case containing __builtin_unreachable leads to wild branch

[Richard Biener <rguenther@suse.de>]

On May 8, 2017 6:41:01 PM GMT+02:00, Peter Bergner <bergner@vnet.ibm.com> wrote:
>On 05/03/2017 08:32 AM, Richard Biener wrote:
> > As for Bernhards concern I share this -- please intead make the
> > interface take either a gimple_seq or a gimple_stmt_iterator
> > instead of a basic-block.  That makes it more obvious you
> > can't use things like gsi_after_labels.  Also I think it's more
> > natural to work backwards as the last stmt in the sequence
> > _has_ to be __builtin_unreachable () and thus checking that first
> > is the cheapest thing to do given that in most cases it will
> > not be __builtin_unreachable () (but a noreturn call or an
> > inifinite loop).
> >
> > Thus, name it gimple_seq_unreachable_p.
>
>So you mean something like the following?

Yes.

>
>/* Returns true if the sequence of statements STMTS only contains
>    a call to __builtin_unreachable ().  */
>
>bool
>gimple_seq_unreachable_p (gimple_seq stmts)
>{
>   gimple_stmt_iterator gsi = gsi_last (stmts);
>
>   if (!gimple_call_builtin_p (gsi_stmt (gsi), BUILT_IN_UNREACHABLE))
>     return false;
>
>   for (gsi_prev (&gsi); !gsi_end_p (gsi); gsi_prev (&gsi))
>     {
>       gimple *stmt = gsi_stmt (gsi);
>       if (gimple_code (stmt) != GIMPLE_LABEL
>           && !is_gimple_debug (stmt)
>           && !gimple_clobber_p (stmt))
>       return false;
>     }
>   return true;
>}
>
>
>
> > On Wed, 26 Apr 2017, Peter Bergner wrote:
> >> One difference from the last patch is that I am no longer setting
> >> default_label to NULL when we emit a decision tree.  I noticed that
> >> the decision tree code seemed to generate slightly better code for
> >> some of my unit tests if I left it alone.  This simplified the
> >> patch somewhat by removing the changes to emit_case_nodes().
>[snip]
> >
> > Can you do the gimple_unreachable_bb_p check earlier in
> > expand_case so it covers the emit_case_decision_tree path as well
> > (and verify that works, of course)?  So basically right at
> >
> >   /* Find the default case target label.  */
> >   default_label = jump_target_rtx
> >       (CASE_LABEL (gimple_switch_default_label (stmt)));
> >   edge default_edge = EDGE_SUCC (bb, 0);
> >   int default_prob = default_edge->probability;
> >
> > handle this case.
>
>That is what the previous patch did, but as I mention above,
>we generate slightly better code for some test cases (other
>tests seemed to generate the same code) if we don't attempt
>to handle the decision tree case.  I'll note that the current
>unpatched compiler already knows how to remove unreachable
>case statement blocks when we expand to a decision tree.
>
>I can add that code back if you think that it will have a
>positive benefit for some test case I haven't tried yet.
>
>Peter