Hi Richard,
On 6 September 2016 at 19:08, Richard Biener <richard.guent...@gmail.com> wrote:
> On Tue, Sep 6, 2016 at 2:24 AM, Kugan Vivekanandarajah
> <kugan.vivekanandara...@linaro.org> wrote:
>> Hi Richard,
>>
>> On 5 September 2016 at 17:57, Richard Biener <richard.guent...@gmail.com>
>> wrote:
>>> On Mon, Sep 5, 2016 at 7:26 AM, Kugan Vivekanandarajah
>>> <kugan.vivekanandara...@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 <kug...@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 <kug...@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 --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 &);
