On Fri, Sep 1, 2017 at 3:01 PM, Martin Liška <mli...@suse.cz> wrote:
> On 09/01/2017 12:57 PM, Richard Biener wrote:
>> On Fri, Sep 1, 2017 at 12:44 PM, Martin Liška <mli...@suse.cz> wrote:
>>> On 09/01/2017 10:26 AM, Richard Biener wrote:
>>>> On Fri, Sep 1, 2017 at 10:07 AM, Martin Liška <mli...@suse.cz> wrote:
>>>>> On 08/30/2017 02:56 PM, Richard Biener wrote:
>>>>>> On Wed, Aug 30, 2017 at 2:32 PM, Martin Liška <mli...@suse.cz> wrote:
>>>>>>> On 08/30/2017 02:28 PM, Richard Biener wrote:
>>>>>>>> On Wed, Aug 30, 2017 at 1:13 PM, Martin Liška <mli...@suse.cz> wrote:
>>>>>>>>> Hi.
>>>>>>>>>
>>>>>>>>> Simple transformation of switch statements where degenerated switch
>>>>>>>>> can be interpreted
>>>>>>>>> as gimple condition (or removed if having any non-default case). I
>>>>>>>>> originally though
>>>>>>>>> that we don't have switch statements without non-default cases, but
>>>>>>>>> PR82032 shows we
>>>>>>>>> can see it.
>>>>>>>>>
>>>>>>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression
>>>>>>>>> tests.
>>>>>>>>>
>>>>>>>>> Ready to be installed?
>>>>>>>>
>>>>>>>> While I guess this case is ok to handle here it would be nice if CFG
>>>>>>>> cleanup
>>>>>>>> would do the same. I suppose find_taken_edge somehow doesn't work for
>>>>>>>> this case even after my last enhancement? Or is CFG cleanup for some
>>>>>>>> reason
>>>>>>>> not run?
>>>>>>>
>>>>>>> Do you mean both with # of non-default edges equal to 0 and 1?
>>>>>>> Let me take a look.
>>>>>>
>>>>>> First and foremost 0. The case of 1 non-default and a default would
>>>>>> need extra code.
>>>>>
>>>>> For the test-case I reduced, one needs:
>>>>>
>>>>> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
>>>>> index b7593068ea9..13af516c6ac 100644
>>>>> --- a/gcc/tree-cfg.c
>>>>> +++ b/gcc/tree-cfg.c
>>>>> @@ -8712,7 +8712,7 @@ const pass_data pass_data_split_crit_edges =
>>>>> PROP_no_crit_edges, /* properties_provided */
>>>>> 0, /* properties_destroyed */
>>>>> 0, /* todo_flags_start */
>>>>> - 0, /* todo_flags_finish */
>>>>> + TODO_cleanup_cfg, /* todo_flags_finish */
>>>>> };
>>>>>
>>>>> class pass_split_crit_edges : public gimple_opt_pass
>>>>>
>>>>> And the code eliminates the problematic switch statement. Do you believe
>>>>> it's the right approach
>>>>> to add the clean up and preserve the assert in tree-switch-conversion.c?
>>>>
>>>> Eh, no. If you run cleanup-cfg after critical edge splitting they
>>>> will be unsplit immediately, making
>>>> it (mostly) a no-op.
>>>>
>>>> OTOH I wanted to eliminate that "pass" in favor of just calling
>>>> split_critical_edges () where needed
>>>> (that's already done in some places).
>>>
>>> Good, so I will leave it to you. Should I in meantime remove the assert in
>>> tree-switch-conversion.c ?
>>
>> Yes, as said your patch was generally OK, I just wondered why we left
>> the switches "unoptimized".
>
> Good.
>
> I'm sending v2 for single non-default case situation.
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>
> Ready to be installed?
Some style nits. generate_high_low_equality is a bit cumbersome I think
and I'd prefer
/* Generate a range test LHS CODE RHS that determines whether INDEX is in the
range [low, high]. Place associated stmts before *GSI. */
void
generate_range_test (gimple_stmt_iterator *gsi, tree index, wide_int
low, wide_int high,
enum tree_code *code, tree *lhs, tree *rhs)
{
}
this captures the implicit requirement of constant low/high (otherwise
you'll generate invalid GIMPLE)
and it makes the comparison code explicit -- otherwise a user has to
inspect users or decipher
the function. Note you'll need to use wi::from (low, TYPE_PRECISION
(TREE_TYPE (index)), SIGNED/UNSIGNED)
to get at the promoted wide-ints/trees.
Otherwise it looks reasonable.
Not super-happy with the tree-cfg.c location for the helper but I
don't have anything more
appropriate either.
Thanks,
Richard.
> Martin
>
>>
>> Richard.
>>
>>>>
>>>>> For the case with # of edges == 1, should I place it to tree-cfg.c in
>>>>> order to trigger it as a clean-up?
>>>>
>>>> I believe the code for edges == 1 can reside in
>>>> cleanup_control_expr_graph. Probably easiest
>>>> from a flow perspective if we do the switch -> cond transform before
>>>> the existing code handling
>>>> cond and switch via find-taken-edge.
>>>
>>> Working on that, good place to do the transformation.
>>>
>>> Martin
>>>
>>>>
>>>>> Thoughts?
>>>>>
>>>>> Martin
>>>>>
>>>>>>
>>>>>> Richard.
>>>>>>
>>>>>>> Martin
>>>>>>>
>>>>>>>>
>>>>>>>> Richard.
>>>>>>>>
>>>>>>>>> Martin
>>>>>>>>>
>>>>>>>>> gcc/ChangeLog:
>>>>>>>>>
>>>>>>>>> 2017-08-25 Martin Liska <mli...@suse.cz>
>>>>>>>>>
>>>>>>>>> PR tree-optimization/82032
>>>>>>>>> * tree-switch-conversion.c (generate_high_low_equality): New
>>>>>>>>> function.
>>>>>>>>> (expand_degenerated_switch): Likewise.
>>>>>>>>> (process_switch): Call expand_degenerated_switch.
>>>>>>>>> (try_switch_expansion): Likewise.
>>>>>>>>> (emit_case_nodes): Use generate_high_low_equality.
>>>>>>>>>
>>>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>>>
>>>>>>>>> 2017-08-25 Martin Liska <mli...@suse.cz>
>>>>>>>>>
>>>>>>>>> PR tree-optimization/82032
>>>>>>>>> * gcc.dg/tree-ssa/pr68198.c: Update jump threading
>>>>>>>>> expectations.
>>>>>>>>> * gcc.dg/tree-ssa/switch-expansion.c: New test.
>>>>>>>>> * gcc.dg/tree-ssa/vrp34.c: Update scanned pattern.
>>>>>>>>> * g++.dg/other/pr82032.C: New test.
>>>>>>>>> ---
>>>>>>>>> gcc/testsuite/g++.dg/other/pr82032.C | 36 +++++++
>>>>>>>>> gcc/testsuite/gcc.dg/tree-ssa/pr68198.c | 6 +-
>>>>>>>>> gcc/testsuite/gcc.dg/tree-ssa/switch-expansion.c | 14 +++
>>>>>>>>> gcc/testsuite/gcc.dg/tree-ssa/vrp34.c | 5 +-
>>>>>>>>> gcc/tree-switch-conversion.c | 123
>>>>>>>>> ++++++++++++++++++-----
>>>>>>>>> 5 files changed, 152 insertions(+), 32 deletions(-)
>>>>>>>>> create mode 100644 gcc/testsuite/g++.dg/other/pr82032.C
>>>>>>>>> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/switch-expansion.c
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>
