On 11/20/20 3:37 PM, Richard Biener wrote:
> On Fri, Nov 20, 2020 at 9:57 AM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 11/19/20 3:46 PM, Richard Biener wrote:
>>> OK, so can you send an updated patch?
> 
> +      tree pos_one = build_int_cst (type, 1);
> +      if (!left->m_has_forward_bb
> +         && !right->m_has_forward_bb
> +         && left->m_case_bb == right->m_case_bb)
> +       {
> +         tree next = int_const_binop (PLUS_EXPR, left->get_high (), pos_one);
> +         if (tree_int_cst_equal (next, right->get_low ()))
> 
> can you please avoid tree arithmetic here and use wide_ints?
> 
>    if (wi::eq (wi::to_wide (right->get_low) - wi::to_wide
> (left->get_high), wi::one (TYPE_PRECISION (type))
> 
> ?
> 
> +      info.record_phi_mapping (info.m_true_edge,
> +                              &info.m_true_edge_phi_mapping);
> +      info.record_phi_mapping (info.m_false_edge,
> +                              &info.m_false_edge_phi_mapping);
> 
> you still have this edge mapping stuff, can't you recover the actual
> PHI arguments from the PHIs during code generation?  I see you removed
> the restriction for all-same values, good.
> 
> +unsigned int
> +pass_if_to_switch::execute (function *fun)
> +{
> +  auto_vec<if_chain *> all_candidates;
> +  hash_map<basic_block, condition_info> conditions_in_bbs;
> +
> +  basic_block bb;
> +  FOR_EACH_BB_FN (bb, fun)
> +    find_conditions (bb, &conditions_in_bbs);
> +
> 
> if we didn't find any suitable conditions we can early out
> 
> +  free_dominance_info (CDI_DOMINATORS);
> +
> +  if (!all_candidates.is_empty ())
> +    mark_virtual_operands_for_renaming (fun);
> 
> please do not free dominator info when you did nothing
> (all_candidates.is_empty ()).
> 
> +             gcond *cond = chain->m_entries[0]->m_cond;
> +             expanded_location loc = expand_location (gimple_location (cond));
> +             if (dump_file)
> +               {
> +                 fprintf (dump_file, "Condition chain (at %s:%d) with %d BBs "
> +                          "transformed into a switch statement.\n",
> +                          loc.file, loc.line,
> +                          chain->m_entries.length ());
> +               }
> 
> if you use dump_enabled_p () and dump_printf_loc you can
> use 'cond' as location itself.
> 
> Otherwise looks OK.

Hello.

I fixed all the notes except the PHI mapping map. It's really not simple
to reconstruct PHI nodes during code-gen. Once a BB is removed in transform phase,
I would need to investigate which PHIs have "holes" (missing PHI argument).

Moreover, I do the very similar thing in gcc/tree-switch-conversion.c::record_phi_operand_mapping.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
And I run SPEC2006 benchmarks, it has done 413 transformations.

Martin

> 
> Thanks,
> Richard.
> 
>> Sure.
>>
>> Martin