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

[Peter Bergner]

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?


/* 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