Merge switch statements in tree-cfgcleanup

[Richard Biener]

On Wed, Jul 20, 2016 at 6:26 PM, Bernd Schmidt <bschmidt@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
>