diff mbox

[RFC,SSA] Iterator to visit SSA

Message ID CAELXzTNsRd-rdVCpaWd1gmPfR-jnR2FXJWoQiXmaTRcPWAknWg@mail.gmail.com
State New
Headers show

Commit Message

Kugan Vivekanandarajah Sept. 6, 2016, 9:33 a.m. UTC
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)
>
> ?

For example, with the test patch where loop body also defines i which
is giving error:

../../trunk/gcc/tree-ssa-ter.c: In function ‘temp_expr_table*
new_temp_expr_table(var_map)’:
../../trunk/gcc/tree-ssa-ter.c:206:11: error: redeclaration of ‘int i’
       int i;
           ^
In file included from ../../trunk/gcc/ssa.h:29:0,
                 from ../../trunk/gcc/tree-ssa-ter.c:28:
../../trunk/gcc/tree-ssanames.h:66:17: note: ‘unsigned int i’
previously declared here
   for (unsigned i = 1; SSANAMES (cfun)->iterate (i, &VAR); ++i)
                 ^
../../trunk/gcc/tree-ssa-ter.c:204:3: note: in expansion of macro
‘FOR_EACH_SSA_NAME’
   FOR_EACH_SSA_NAME (name)
   ^
Makefile:1097: recipe for target 'tree-ssa-ter.o' failed

Am I missing something here ?

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.

Comments

Richard Biener Sept. 6, 2016, 9:57 a.m. UTC | #1
On Tue, Sep 6, 2016 at 11:33 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)
>>
>> ?
>
> For example, with the test patch where loop body also defines i which
> is giving error:
>
> ../../trunk/gcc/tree-ssa-ter.c: In function ‘temp_expr_table*
> new_temp_expr_table(var_map)’:
> ../../trunk/gcc/tree-ssa-ter.c:206:11: error: redeclaration of ‘int i’
>        int i;
>            ^
> In file included from ../../trunk/gcc/ssa.h:29:0,
>                  from ../../trunk/gcc/tree-ssa-ter.c:28:
> ../../trunk/gcc/tree-ssanames.h:66:17: note: ‘unsigned int i’
> previously declared here
>    for (unsigned i = 1; SSANAMES (cfun)->iterate (i, &VAR); ++i)
>                  ^
> ../../trunk/gcc/tree-ssa-ter.c:204:3: note: in expansion of macro
> ‘FOR_EACH_SSA_NAME’
>    FOR_EACH_SSA_NAME (name)
>    ^
> Makefile:1097: recipe for target 'tree-ssa-ter.o' failed
>
> Am I missing something here ?

Well, my comment was not about passing 'i' to the macro but about
not being able to skip NULL names.  I just copied & pasted my
earlier macro.

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.
Kugan Vivekanandarajah Sept. 6, 2016, 10:07 a.m. UTC | #2
Hi Richard,


On 6 September 2016 at 19:57, Richard Biener <richard.guenther@gmail.com> wrote:
> On Tue, Sep 6, 2016 at 11:33 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)
>>>
>>> ?
>>
>> For example, with the test patch where loop body also defines i which
>> is giving error:
>>
>> ../../trunk/gcc/tree-ssa-ter.c: In function ‘temp_expr_table*
>> new_temp_expr_table(var_map)’:
>> ../../trunk/gcc/tree-ssa-ter.c:206:11: error: redeclaration of ‘int i’
>>        int i;
>>            ^
>> In file included from ../../trunk/gcc/ssa.h:29:0,
>>                  from ../../trunk/gcc/tree-ssa-ter.c:28:
>> ../../trunk/gcc/tree-ssanames.h:66:17: note: ‘unsigned int i’
>> previously declared here
>>    for (unsigned i = 1; SSANAMES (cfun)->iterate (i, &VAR); ++i)
>>                  ^
>> ../../trunk/gcc/tree-ssa-ter.c:204:3: note: in expansion of macro
>> ‘FOR_EACH_SSA_NAME’
>>    FOR_EACH_SSA_NAME (name)
>>    ^
>> Makefile:1097: recipe for target 'tree-ssa-ter.o' failed
>>
>> Am I missing something here ?
>
> Well, my comment was not about passing 'i' to the macro but about
> not being able to skip NULL names.  I just copied & pasted my
> earlier macro.

Sorry, I misunderstood your comment.

I have:

+#define FOR_EACH_SSA_NAME(i, VAR) \
+  for (i = 1; SSANAMES (cfun)->iterate (i, &VAR); ++i)

We will skip the first (0 index) if that is what you are referring here.

I also thought that you wanted to skip any NULL_TREEs after that too.
After looking at your previous your comment, I don’t you think you
wanted that anyway.

Therefore I think I am doing what you wanted in my last patch.

Thanks,
Kugan


> 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.
Richard Biener Sept. 6, 2016, 10:15 a.m. UTC | #3
On Tue, Sep 6, 2016 at 12:07 PM, Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
>
>
> On 6 September 2016 at 19:57, Richard Biener <richard.guenther@gmail.com> wrote:
>> On Tue, Sep 6, 2016 at 11:33 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)
>>>>
>>>> ?
>>>
>>> For example, with the test patch where loop body also defines i which
>>> is giving error:
>>>
>>> ../../trunk/gcc/tree-ssa-ter.c: In function ‘temp_expr_table*
>>> new_temp_expr_table(var_map)’:
>>> ../../trunk/gcc/tree-ssa-ter.c:206:11: error: redeclaration of ‘int i’
>>>        int i;
>>>            ^
>>> In file included from ../../trunk/gcc/ssa.h:29:0,
>>>                  from ../../trunk/gcc/tree-ssa-ter.c:28:
>>> ../../trunk/gcc/tree-ssanames.h:66:17: note: ‘unsigned int i’
>>> previously declared here
>>>    for (unsigned i = 1; SSANAMES (cfun)->iterate (i, &VAR); ++i)
>>>                  ^
>>> ../../trunk/gcc/tree-ssa-ter.c:204:3: note: in expansion of macro
>>> ‘FOR_EACH_SSA_NAME’
>>>    FOR_EACH_SSA_NAME (name)
>>>    ^
>>> Makefile:1097: recipe for target 'tree-ssa-ter.o' failed
>>>
>>> Am I missing something here ?
>>
>> Well, my comment was not about passing 'i' to the macro but about
>> not being able to skip NULL names.  I just copied & pasted my
>> earlier macro.
>
> Sorry, I misunderstood your comment.
>
> I have:
>
> +#define FOR_EACH_SSA_NAME(i, VAR) \
> +  for (i = 1; SSANAMES (cfun)->iterate (i, &VAR); ++i)
>
> We will skip the first (0 index) if that is what you are referring here.
>
> I also thought that you wanted to skip any NULL_TREEs after that too.
> After looking at your previous your comment, I don’t you think you
> wanted that anyway.

I _did_ want that, otherwise my suggestion to use

 #define FOR_EACH_SSA_NAME(i, VAR) \
   for (i = 1; SSANAMES (cfun)->iterate (i, &VAR); ++i) \
      if (VAR)

would have made no sense.  Btw, please use capital i and put uses
in parens when not lvalues (see FOR_EACH_VEC_ELT).  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)

Thanks,
Richard.

> Therefore I think I am doing what you wanted in my last patch.
>
> Thanks,
> Kugan
>
>
>> 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.
diff mbox

Patch

diff --git a/gcc/tree-ssa-ter.c b/gcc/tree-ssa-ter.c
index 2a772b2..15f30ee 100644
--- a/gcc/tree-ssa-ter.c
+++ b/gcc/tree-ssa-ter.c
@@ -185,7 +185,7 @@  extern void debug_ter (FILE *, temp_expr_table *);
 static temp_expr_table *
 new_temp_expr_table (var_map map)
 {
-  unsigned x;
+  tree name;
 
   temp_expr_table *t = XNEW (struct temp_expr_table);
   t->map = map;
@@ -201,15 +201,14 @@  new_temp_expr_table (var_map map)
 
   t->replaceable_expressions = NULL;
   t->num_in_part = XCNEWVEC (int, num_var_partitions (map));
-  for (x = 1; x < num_ssa_names; x++)
+  FOR_EACH_SSA_NAME (name)
     {
-      int p;
-      tree name = ssa_name (x);
+      int i;
       if (!name)
         continue;
-      p = var_to_partition (map, name);
-      if (p != NO_PARTITION)
-        t->num_in_part[p]++;
+      i = var_to_partition (map, name);
+      if (i != NO_PARTITION)
+        t->num_in_part[i]++;
     }
   t->call_cnt = XCNEWVEC (int, num_ssa_names + 1);
 
diff --git a/gcc/tree-ssanames.h b/gcc/tree-ssanames.h
index 8e66ce6..538e35f 100644
--- a/gcc/tree-ssanames.h
+++ b/gcc/tree-ssanames.h
@@ -62,6 +62,9 @@  struct GTY ((variable_size)) range_info_def {
 #define num_ssa_names (vec_safe_length (cfun->gimple_df->ssa_names))
 #define ssa_name(i) ((*cfun->gimple_df->ssa_names)[(i)])
 
+#define FOR_EACH_SSA_NAME(VAR) \
+  for (unsigned i = 1; SSANAMES (cfun)->iterate (i, &VAR); ++i)
+
 /* Sets the value range to SSA.  */
 extern void set_range_info (tree, enum value_range_type, const wide_int_ref &,
 			    const wide_int_ref &);