Re: [RFC][SSA] Iterator to visit SSA

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Sep 7, 2016 at 2:21 AM, Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
>
> On 6 September 2016 at 19:08, Richard Biener <richard.guenther@gmail.com> wrote:
>> On Tue, Sep 6, 2016 at 2:24 AM, Kugan Vivekanandarajah
>> <kugan.vivekanandarajah@linaro.org> wrote:
>>> Hi Richard,
>>>
>>> On 5 September 2016 at 17:57, Richard Biener <richard.guenther@gmail.com> wrote:
>>>> On Mon, Sep 5, 2016 at 7:26 AM, Kugan Vivekanandarajah
>>>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>>> Hi All,
>>>>>
>>>>> While looking at gcc source, I noticed that we are iterating over SSA
>>>>> variable from 0 to num_ssa_names in some cases and 1 to num_ssa_names
>>>>> in others. It seems that variable 0 is always NULL TREE.
>>>>
>>>> Yeah, that's artificial because we don't assign SSA name version 0 (for some
>>>> unknown reason).
>>>>
>>>>> But it can
>>>>> confuse people who are looking for the first time. Therefore It might
>>>>> be good to follow some consistent usage here.
>>>>>
>>>>> It might be also good to gave a FOR_EACH_SSAVAR iterator as we do in
>>>>> other case. Here is attempt to do this based on what is done in other
>>>>> places. Bootstrapped and regression tested on X86_64-linux-gnu with no
>>>>> new regressions. is this OK?
>>>>
>>>> First of all some bikeshedding - FOR_EACH_SSA_NAME would be better
>>>> as SSAVAR might be confused with iterating over SSA_NAME_VAR.
>>>>
>>>> Then, if you add an iterator why leave the name == NULL handling to consumers?
>>>> That looks odd.
>>>>
>>>> Then, SSA names are just in a vector thus why not re-use the vector iterator?
>>>>
>>>> That is,
>>>>
>>>> #define FOR_EACH_SSA_NAME (name) \
>>>>   for (unsigned i = 1; SSANAMES (cfun).iterate (i, &name); ++i)
>>>>
>>>> would be equivalent to your patch?
>>>>
>>>> Please also don't add new iterators that implicitely use 'cfun' but always use
>>>> one that passes it as explicit arg.
>>>
>>> I think defining FOR_EACH_SSA_NAME with vector iterator is better. But
>>> we will not be able to skill NULL ssa_names with that.
>>
>> Why?  Can't you simply do
>>
>>   #define FOR_EACH_SSA_NAME (name) \
>>     for (unsigned i = 1; SSANAMES (cfun).iterate (i, &name); ++i) \
>>        if (name)
>>
>> ?
>
> Indeed.  I missed the if at the end :(.  Here is an updated patch.
> Bootstrapped and regression tested on x86_64-linux-gnu with no new
> regressions.

Quoting myself:

> As I said
> I'd like 'cfun' to be explicit, thus
>
> #define FOR_EACH_SSA_NAME(I, VAR, FN) \
>  for (I = 1; SSANAMES (FN)->iterate ((I), &(VAR)); ++(I)) \
>   if (VAR)

the patch doesn't seem to contain that FN part.  Please also put declarations
of 'name' you need to add right before the FOR_EACH_SSA_NAME rather
than far away.

Thanks,
Richard.

> Thanks,
> Kugan
>>
>>> I also added
>>> index variable to the macro so that there want be any conflicts if the
>>> index variable "i" (or whatever) is also defined in the loop.
>>>
>>> Bootstrapped and regression tested on x86_64-linux-gnu with no new
>>> regressions. Is this OK for trunk?
>>>
>>> Thanks,
>>> Kugan
>>>
>>>
>>> gcc/ChangeLog:
>>>
>>> 2016-09-06  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>
>>>     * tree-ssanames.h (FOR_EACH_SSA_NAME): New.
>>>     * cfgexpand.c (update_alias_info_with_stack_vars): Use
>>>     FOR_EACH_SSA_NAME to iterate over SSA variables.
>>>     (pass_expand::execute): Likewise.
>>>     * omp-simd-clone.c (ipa_simd_modify_function_body): Likewise.
>>>     * tree-cfg.c (dump_function_to_file): Likewise.
>>>     * tree-into-ssa.c (pass_build_ssa::execute): Likewise.
>>>     (update_ssa): Likewise.
>>>     * tree-ssa-alias.c (dump_alias_info): Likewise.
>>>     * tree-ssa-ccp.c (ccp_finalize): Likewise.
>>>     * tree-ssa-coalesce.c (build_ssa_conflict_graph): Likewise.
>>>     (create_outofssa_var_map): Likewise.
>>>     (coalesce_ssa_name): Likewise.
>>>     * tree-ssa-operands.c (dump_immediate_uses): Likewise.
>>>     * tree-ssa-pre.c (compute_avail): Likewise.
>>>     * tree-ssa-sccvn.c (init_scc_vn): Likewise.
>>>     (scc_vn_restore_ssa_info): Likewise.
>>>     (free_scc_vn): Likwise.
>>>     (run_scc_vn): Likewise.
>>>     * tree-ssa-structalias.c (compute_points_to_sets): Likewise.
>>>     * tree-ssa-ter.c (new_temp_expr_table): Likewise.
>>>     * tree-ssa-copy.c (fini_copy_prop): Likewise.
>>>     * tree-ssa.c (verify_ssa): Likewise.
>>>
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>
>>>>> Thanks,
>>>>> Kugan
>>>>>
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>> 2016-09-05  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>>>
>>>>>     * tree-ssanames.h (ssa_iterator::ssa_iterator): New.
>>>>>     (ssa_iterator::get): Likewise.
>>>>>     (ssa_iterator::next): Likewise.
>>>>>     (FOR_EACH_SSAVAR): Likewise.
>>>>>     * cfgexpand.c (update_alias_info_with_stack_vars): Use
>>>>>     FOR_EACH_SSAVAR to iterate over SSA variables.
>>>>>     (pass_expand::execute): Likewise.
>>>>>     * omp-simd-clone.c (ipa_simd_modify_function_body): Likewise.
>>>>>     * tree-cfg.c (dump_function_to_file): Likewise.
>>>>>     * tree-into-ssa.c (pass_build_ssa::execute): Likewise.
>>>>>     (update_ssa): Likewise.
>>>>>     * tree-ssa-alias.c (dump_alias_info): Likewise.
>>>>>     * tree-ssa-ccp.c (ccp_finalize): Likewise.
>>>>>     * tree-ssa-coalesce.c (build_ssa_conflict_graph): Likewise.
>>>>>     (create_outofssa_var_map): Likewise.
>>>>>     (coalesce_ssa_name): Likewise.
>>>>>     * tree-ssa-operands.c (dump_immediate_uses): Likewise.
>>>>>     * tree-ssa-pre.c (compute_avail): Likewise.
>>>>>     * tree-ssa-sccvn.c (init_scc_vn): Likewise.
>>>>>     (scc_vn_restore_ssa_info): Likewise.
>>>>>     (free_scc_vn): Likwise.
>>>>>     (run_scc_vn): Likewise.
>>>>>     * tree-ssa-structalias.c (compute_points_to_sets): Likewise.
>>>>>     * tree-ssa-ter.c (new_temp_expr_table): Likewise.