On Wed, Jul 20, 2016 at 6:26 PM, Bernd Schmidt <bschm...@redhat.com> wrote: > On 07/20/2016 06:09 PM, Jeff Law wrote: >> >> So I'm going to let Richi run with the review on this one since the two >> of you are already iterating. But I did have one comment on the >> placement of the pass. >> >> I believe one of the key things to consider for whether or not something >> like this belongs in the cfgcleanup code is whether or not the >> optimization is likely exposed repeatedly through the optimization >> pipeline. If it's mostly a source level issue or only usually exposed >> by a limited set of optimizers, then a separate pass might be better. > > > It can trigger before switchconv, and could expose optimization > opportunities there, but I've also seen it trigger much later. Since I think > it's cheap I don't see a reason not to put it in cfgcleanup, IMO it's the > best fit conceptually.
I'm not convinced. We do have CFG matching passes like phi-opt, if-combine. With your logic even simple jump-threading like if (a) if (a) ... would be a job for CFG cleanup? + gsi2 = gsi_start_nondebug_after_labels_bb (bb); + if (gsi_end_p (gsi2)) + return false; + + gimple *stmt = gsi_stmt (gsi2); + if (gimple_code (stmt) != GIMPLE_SWITCH) + return false; stmt = last_stmt (bb); if (!stmt || gimple_code (stmt) != GIMPLE_SWITHC) return false; is shorter and has the same effect. + edge e; + edge_iterator ei; + FOR_EACH_EDGE (e, ei, bb->succs) + { + basic_block dest = e->dest; + if (find_edge (pred_bb, dest)) + { + /* If a destination block is reached from both switches, any + phi nodes there would become corrupted. */ + gphi_iterator psi = gsi_start_phis (dest); + if (!gsi_end_p (psi)) + return false; + } + } I wonder if this tests all required blocks. Why can't there be a forwarder between pred_bb and dest for example? I think you want sth like if (! dominated_by_p (CDI_DOMINATORS, dest, bb)) ... verify no PHIs ... + auto_vec<tree> new_labels; + please pre-reserve enough space to hold labels from both switches and use quick_push below. + /* Merge adjacent ranges. */ + size_t new_count = new_labels.length (); + for (i1 = i2 = 1; i1 < new_count; ) + { + ce1 = new_labels[i1++]; + tree high = CASE_HIGH (ce1); + if (high == NULL_TREE) + high = CASE_LOW (ce1); + while (i1 < new_count) + { + ce2 = new_labels[i1]; what about re-using the code from tree-cfg.c instead? (group_case_labels_stmt) + if (dom_info_available_p (CDI_DOMINATORS)) I think DOM info is always available during CFG cleanup + { + basic_block dombb = get_immediate_dominator (CDI_DOMINATORS, bb); + gcc_assert (dombb == pred_bb); + vec<basic_block> fix_bbs; + fix_bbs = get_all_dominated_blocks (CDI_DOMINATORS, pred_bb); + iterate_fix_dominators (CDI_DOMINATORS, fix_bbs, false); + fix_bbs.release (); + } Richard. > > Bernd >