Re: [PATCH 3/3] Enhance dumps of IVOPTS
On Fri, May 13, 2016 at 7:17 AM, H.J. Lu wrote:
> On Fri, May 13, 2016 at 5:51 AM, Martin Liška wrote:
>> On 05/13/2016 02:46 PM, Richard Biener wrote:
>>> Use them for HOST_WIDE_INT printing, for [u]int64_t use the PRI stuff.
>>>
>>> Richard.
>>
>> Thanks you both, installed as r236208.
>>
>
> It isn't fixed:
>
> /export/gnu/import/git/sources/gcc/gcc/tree-ssa-loop-ivopts.c:7052:41:
> error: format ‘%llu’ expects argument of type ‘long long unsigned
> int’, but argument 3 has type ‘size_t {aka unsigned int}’
> [-Werror=format=]
> set->used_inv_exprs->elements ());
>
I am going to check in this as an obvious fix.
--
H.J.
---
diff --git a/gcc/tree-ssa-loop-ivopts.c b/gcc/tree-ssa-loop-ivopts.c
index 2b2115f..e8953a0 100644
--- a/gcc/tree-ssa-loop-ivopts.c
+++ b/gcc/tree-ssa-loop-ivopts.c
@@ -7048,8 +7048,8 @@ create_new_ivs (struct ivopts_data *data, struct
iv_ca *set)
LOCATION_LINE (data->loop_loc));
fprintf (dump_file, ", " HOST_WIDE_INT_PRINT_DEC " avg niters",
avg_loop_niter (data->current_loop));
- fprintf (dump_file, ", %" PRIu64 " expressions",
- set->used_inv_exprs->elements ());
+ fprintf (dump_file, ", " HOST_WIDE_INT_PRINT_UNSIGNED " expressions",
+ (unsigned HOST_WIDE_INT) set->used_inv_exprs->elements ());
fprintf (dump_file, ", %lu IVs:\n", bitmap_count_bits (set->cands));
EXECUTE_IF_SET_IN_BITMAP (set->cands, 0, i, bi)
{
Re: [PATCH 3/3] Enhance dumps of IVOPTS
On Fri, May 13, 2016 at 5:51 AM, Martin Liška wrote:
> On 05/13/2016 02:46 PM, Richard Biener wrote:
>> Use them for HOST_WIDE_INT printing, for [u]int64_t use the PRI stuff.
>>
>> Richard.
>
> Thanks you both, installed as r236208.
>
It isn't fixed:
/export/gnu/import/git/sources/gcc/gcc/tree-ssa-loop-ivopts.c:7052:41:
error: format ‘%llu’ expects argument of type ‘long long unsigned
int’, but argument 3 has type ‘size_t {aka unsigned int}’
[-Werror=format=]
set->used_inv_exprs->elements ());
--
H.J.
Re: [PATCH 3/3] Enhance dumps of IVOPTS
On 05/13/2016 02:46 PM, Richard Biener wrote: > Use them for HOST_WIDE_INT printing, for [u]int64_t use the PRI stuff. > > Richard. Thanks you both, installed as r236208. Martin
Re: [PATCH 3/3] Enhance dumps of IVOPTS
On Fri, May 13, 2016 at 2:43 PM, Kyrill Tkachov
wrote:
> Hi Martin,
>
>
> On 13/05/16 13:39, Martin Liška wrote:
>>
>> On 05/13/2016 02:11 PM, H.J. Lu wrote:
>>>
>>> On Fri, May 13, 2016 at 3:44 AM, Martin Liška wrote:
On 05/13/2016 11:43 AM, Bin.Cheng wrote:
>
> On Thu, May 12, 2016 at 5:41 PM, Martin Liška wrote:
>>
>> On 05/12/2016 03:51 PM, Bin.Cheng wrote:
>>>
>>> On Thu, May 12, 2016 at 1:13 PM, Martin Liška wrote:
On 05/10/2016 03:16 PM, Bin.Cheng wrote:
>
> Another way is to remove the use of id for struct iv_inv_expr_ent
> once
> for all. We can change iv_ca.used_inv_expr and
> cost_pair.inv_expr_id
> to pointers, and rename iv_inv_expr_ent.id to count and use this to
> record reference number in iv_ca. This if-statement on dump_file
> can
> be saved. Also I think it simplifies current code a bit. For now,
> there are id <-> struct maps for different structures in IVOPT
> which
> make it not straightforward.
Hi.
I'm sending second version of the patch. I tried to follow your
advices, but
because of a iv_inv_expr_ent can simultaneously belong to multiply
iv_cas,
putting counter to iv_inv_expr_ent does not works. Instead of that,
I've
decided to replace used_inv_expr with a hash_map that contains used
inv_exps
and where value of the map is # of usages.
Further questions:
+ iv_inv_expr_ent::id can be now removed as it's used just for
purpose of dumps
Group 0:
cand costscaled freqcompl. depends on
5 2 2.001.000
6 4 4.001.001inv_expr:0
7 4 4.001.001inv_expr:1
8 4 4.001.001inv_expr:2
That can be replaced with print_generic_expr, but I think using ids
makes the dump
output more clear.
>>>
>>> I am okay with keeping id. Could you please dump all inv_exprs in a
>>> single section like
>>> :
>>> inv_expr 0: print_generic_expr
>>> inv_expr 1: ...
>>>
>>> Then only dump the id afterwards?
>>>
>> Sure, it would be definitely better:
>>
>> The new dump format looks:
>>
>> :
>> inv_expr 0: sudoku_351(D) + (sizetype) S.833_774 * 4
>> inv_expr 1: sudoku_351(D) + ((sizetype) S.833_774 * 4 +
>> 18446744073709551580)
>> inv_expr 2: sudoku_351(D) + ((sizetype) S.833_774 + 72) * 4
>> inv_expr 3: sudoku_351(D) + ((sizetype) S.833_774 + 81) * 4
>> inv_expr 4: &A.832 + (sizetype) _377 * 4
>> inv_expr 5: &A.832 + ((sizetype) _377 * 4 + 18446744073709551612)
>> inv_expr 6: &A.832 + ((sizetype) _377 + 8) * 4
>> inv_expr 7: &A.832 + ((sizetype) _377 + 9) * 4
>>
>> :
>> Group 0:
>>cand costscaled freqcompl. depends on
>>
>> ...
>>
>> Improved to:
>>cost: 27 (complexity 2)
>>cand_cost: 11
>>cand_group_cost: 10 (complexity 2)
>>candidates: 3, 5
>> group:0 --> iv_cand:5, cost=(2,0)
>> group:1 --> iv_cand:5, cost=(4,1)
>> group:2 --> iv_cand:5, cost=(4,1)
>> group:3 --> iv_cand:3, cost=(0,0)
>> group:4 --> iv_cand:3, cost=(0,0)
>>invariants 1, 6
>>invariant expressions 6, 3
>>
>> The only question here is that as used_inv_exprs are stored in a
>> hash_map,
>> order of dumped invariants would not be stable. Is it problem?
>
> It is okay.
>
> Only nitpicking on this version.
>
+ As check_GNU_style.sh reported multiple 8 spaces issues in hunks
I've touched, I decided
to fix all 8 spaces issues. Hope it's fine.
I'm going to test the patch.
Thoughts?
>>>
>>> Some comments on the patch embedded.
>>>
+/* Forward declaration. */
>>>
>>> Not necessary.
+struct iv_inv_expr_ent;
+
>>
>> I think it's needed because struct cost_pair uses a pointer to
>> iv_inv_expr_ent.
>
> I mean the comment, clearly the declaration is self-documented.
Hi.
Yeah, removed.
>> @@ -6000,11 +6045,12 @@ iv_ca_set_no_cp (struct ivopts_data *data,
>> struct iv_ca *ivs,
>>
>> iv_ca_set_remove_invariants (ivs, cp->depends_on);
>>
>> - if (cp->inv_expr_id != -1)
>> + if (cp->inv_expr != NULL)
>> {
>> - ivs->used_inv_expr[cp->inv_expr_id]--;
>> - if (ivs->used_inv_expr[cp->inv_expr_id] == 0)
>> -ivs->num_used_inv_expr--;
>> + unsigned *slot = ivs->used_inv_exprs->get (cp->inv_expr);
>> + --(*slot);
>> + if (*slot == 0)
>> +ivs->used_inv_exprs->re
Re: [PATCH 3/3] Enhance dumps of IVOPTS
Hi Martin,
On 13/05/16 13:39, Martin Liška wrote:
On 05/13/2016 02:11 PM, H.J. Lu wrote:
On Fri, May 13, 2016 at 3:44 AM, Martin Liška wrote:
On 05/13/2016 11:43 AM, Bin.Cheng wrote:
On Thu, May 12, 2016 at 5:41 PM, Martin Liška wrote:
On 05/12/2016 03:51 PM, Bin.Cheng wrote:
On Thu, May 12, 2016 at 1:13 PM, Martin Liška wrote:
On 05/10/2016 03:16 PM, Bin.Cheng wrote:
Another way is to remove the use of id for struct iv_inv_expr_ent once
for all. We can change iv_ca.used_inv_expr and cost_pair.inv_expr_id
to pointers, and rename iv_inv_expr_ent.id to count and use this to
record reference number in iv_ca. This if-statement on dump_file can
be saved. Also I think it simplifies current code a bit. For now,
there are id <-> struct maps for different structures in IVOPT which
make it not straightforward.
Hi.
I'm sending second version of the patch. I tried to follow your advices, but
because of a iv_inv_expr_ent can simultaneously belong to multiply iv_cas,
putting counter to iv_inv_expr_ent does not works. Instead of that, I've
decided to replace used_inv_expr with a hash_map that contains used inv_exps
and where value of the map is # of usages.
Further questions:
+ iv_inv_expr_ent::id can be now removed as it's used just for purpose of dumps
Group 0:
cand costscaled freqcompl. depends on
5 2 2.001.000
6 4 4.001.001inv_expr:0
7 4 4.001.001inv_expr:1
8 4 4.001.001inv_expr:2
That can be replaced with print_generic_expr, but I think using ids makes the
dump
output more clear.
I am okay with keeping id. Could you please dump all inv_exprs in a
single section like
:
inv_expr 0: print_generic_expr
inv_expr 1: ...
Then only dump the id afterwards?
Sure, it would be definitely better:
The new dump format looks:
:
inv_expr 0: sudoku_351(D) + (sizetype) S.833_774 * 4
inv_expr 1: sudoku_351(D) + ((sizetype) S.833_774 * 4 +
18446744073709551580)
inv_expr 2: sudoku_351(D) + ((sizetype) S.833_774 + 72) * 4
inv_expr 3: sudoku_351(D) + ((sizetype) S.833_774 + 81) * 4
inv_expr 4: &A.832 + (sizetype) _377 * 4
inv_expr 5: &A.832 + ((sizetype) _377 * 4 + 18446744073709551612)
inv_expr 6: &A.832 + ((sizetype) _377 + 8) * 4
inv_expr 7: &A.832 + ((sizetype) _377 + 9) * 4
:
Group 0:
cand costscaled freqcompl. depends on
...
Improved to:
cost: 27 (complexity 2)
cand_cost: 11
cand_group_cost: 10 (complexity 2)
candidates: 3, 5
group:0 --> iv_cand:5, cost=(2,0)
group:1 --> iv_cand:5, cost=(4,1)
group:2 --> iv_cand:5, cost=(4,1)
group:3 --> iv_cand:3, cost=(0,0)
group:4 --> iv_cand:3, cost=(0,0)
invariants 1, 6
invariant expressions 6, 3
The only question here is that as used_inv_exprs are stored in a hash_map,
order of dumped invariants would not be stable. Is it problem?
It is okay.
Only nitpicking on this version.
+ As check_GNU_style.sh reported multiple 8 spaces issues in hunks I've
touched, I decided
to fix all 8 spaces issues. Hope it's fine.
I'm going to test the patch.
Thoughts?
Some comments on the patch embedded.
+/* Forward declaration. */
Not necessary.
+struct iv_inv_expr_ent;
+
I think it's needed because struct cost_pair uses a pointer to iv_inv_expr_ent.
I mean the comment, clearly the declaration is self-documented.
Hi.
Yeah, removed.
@@ -6000,11 +6045,12 @@ iv_ca_set_no_cp (struct ivopts_data *data, struct iv_ca
*ivs,
iv_ca_set_remove_invariants (ivs, cp->depends_on);
- if (cp->inv_expr_id != -1)
+ if (cp->inv_expr != NULL)
{
- ivs->used_inv_expr[cp->inv_expr_id]--;
- if (ivs->used_inv_expr[cp->inv_expr_id] == 0)
-ivs->num_used_inv_expr--;
+ unsigned *slot = ivs->used_inv_exprs->get (cp->inv_expr);
+ --(*slot);
+ if (*slot == 0)
+ivs->used_inv_exprs->remove (cp->inv_expr);
I suppose insertion/removal of hash_map are not expensive? Because
the algorithm causes a lot of these operations.
I think it should be ~ a constant operation.
@@ -6324,12 +6368,26 @@ iv_ca_dump (struct ivopts_data *data, FILE *file,
struct iv_ca *ivs)
fprintf (file, " group:%d --> ??\n", group->id);
}
+ bool any_invariant = false;
for (i = 1; i <= data->max_inv_id; i++)
if (ivs->n_invariant_uses[i])
{
+const char *pref = any_invariant ? ", " : " invariants ";
+any_invariant = true;
fprintf (file, "%s%d", pref, i);
-pref = ", ";
}
+
+ if (any_invariant)
+fprintf (file, "\n");
+
To make dump easier to read, we can simply dump invariant
variables/expressions unconditionally. Also keep invariant variables
and expressions in the same form.
Sure, that's a good idea!
Sample output:
Initial set of candidates:
cost: 17 (complexity 0)
cand_cost: 11
cand_group_cost: 2 (complexity 0)
candidates: 1, 5
group:0 --> iv_cand:5, cost=(2,0)
group:1 --> iv_cand:1, cost=(0,0)
invariant variabl
Re: [PATCH 3/3] Enhance dumps of IVOPTS
On 05/13/2016 02:11 PM, H.J. Lu wrote:
> On Fri, May 13, 2016 at 3:44 AM, Martin Liška wrote:
>> On 05/13/2016 11:43 AM, Bin.Cheng wrote:
>>> On Thu, May 12, 2016 at 5:41 PM, Martin Liška wrote:
On 05/12/2016 03:51 PM, Bin.Cheng wrote:
> On Thu, May 12, 2016 at 1:13 PM, Martin Liška wrote:
>> On 05/10/2016 03:16 PM, Bin.Cheng wrote:
>>> Another way is to remove the use of id for struct iv_inv_expr_ent once
>>> for all. We can change iv_ca.used_inv_expr and cost_pair.inv_expr_id
>>> to pointers, and rename iv_inv_expr_ent.id to count and use this to
>>> record reference number in iv_ca. This if-statement on dump_file can
>>> be saved. Also I think it simplifies current code a bit. For now,
>>> there are id <-> struct maps for different structures in IVOPT which
>>> make it not straightforward.
>>
>> Hi.
>>
>> I'm sending second version of the patch. I tried to follow your advices,
>> but
>> because of a iv_inv_expr_ent can simultaneously belong to multiply
>> iv_cas,
>> putting counter to iv_inv_expr_ent does not works. Instead of that, I've
>> decided to replace used_inv_expr with a hash_map that contains used
>> inv_exps
>> and where value of the map is # of usages.
>>
>> Further questions:
>> + iv_inv_expr_ent::id can be now removed as it's used just for purpose
>> of dumps
>> Group 0:
>> cand costscaled freqcompl. depends on
>> 5 2 2.001.000
>> 6 4 4.001.001inv_expr:0
>> 7 4 4.001.001inv_expr:1
>> 8 4 4.001.001inv_expr:2
>>
>> That can be replaced with print_generic_expr, but I think using ids
>> makes the dump
>> output more clear.
> I am okay with keeping id. Could you please dump all inv_exprs in a
> single section like
> :
> inv_expr 0: print_generic_expr
> inv_expr 1: ...
>
> Then only dump the id afterwards?
>
Sure, it would be definitely better:
The new dump format looks:
:
inv_expr 0: sudoku_351(D) + (sizetype) S.833_774 * 4
inv_expr 1: sudoku_351(D) + ((sizetype) S.833_774 * 4 +
18446744073709551580)
inv_expr 2: sudoku_351(D) + ((sizetype) S.833_774 + 72) * 4
inv_expr 3: sudoku_351(D) + ((sizetype) S.833_774 + 81) * 4
inv_expr 4: &A.832 + (sizetype) _377 * 4
inv_expr 5: &A.832 + ((sizetype) _377 * 4 + 18446744073709551612)
inv_expr 6: &A.832 + ((sizetype) _377 + 8) * 4
inv_expr 7: &A.832 + ((sizetype) _377 + 9) * 4
:
Group 0:
cand costscaled freqcompl. depends on
...
Improved to:
cost: 27 (complexity 2)
cand_cost: 11
cand_group_cost: 10 (complexity 2)
candidates: 3, 5
group:0 --> iv_cand:5, cost=(2,0)
group:1 --> iv_cand:5, cost=(4,1)
group:2 --> iv_cand:5, cost=(4,1)
group:3 --> iv_cand:3, cost=(0,0)
group:4 --> iv_cand:3, cost=(0,0)
invariants 1, 6
invariant expressions 6, 3
The only question here is that as used_inv_exprs are stored in a hash_map,
order of dumped invariants would not be stable. Is it problem?
>>> It is okay.
>>>
>>> Only nitpicking on this version.
>>>
>>
>> + As check_GNU_style.sh reported multiple 8 spaces issues in hunks I've
>> touched, I decided
>> to fix all 8 spaces issues. Hope it's fine.
>>
>> I'm going to test the patch.
>> Thoughts?
>
> Some comments on the patch embedded.
>
>>
>> +/* Forward declaration. */
> Not necessary.
>> +struct iv_inv_expr_ent;
>> +
I think it's needed because struct cost_pair uses a pointer to
iv_inv_expr_ent.
>>> I mean the comment, clearly the declaration is self-documented.
>>
>> Hi.
>>
>> Yeah, removed.
>>
>>>
@@ -6000,11 +6045,12 @@ iv_ca_set_no_cp (struct ivopts_data *data, struct
iv_ca *ivs,
iv_ca_set_remove_invariants (ivs, cp->depends_on);
- if (cp->inv_expr_id != -1)
+ if (cp->inv_expr != NULL)
{
- ivs->used_inv_expr[cp->inv_expr_id]--;
- if (ivs->used_inv_expr[cp->inv_expr_id] == 0)
-ivs->num_used_inv_expr--;
+ unsigned *slot = ivs->used_inv_exprs->get (cp->inv_expr);
+ --(*slot);
+ if (*slot == 0)
+ivs->used_inv_exprs->remove (cp->inv_expr);
>>> I suppose insertion/removal of hash_map are not expensive? Because
>>> the algorithm causes a lot of these operations.
>>
>> I think it should be ~ a constant operation.
>>
>>>
@@ -6324,12 +6368,26 @@ iv_ca_dump (struct ivopts_data *data, FILE *file,
struct iv_ca *ivs)
fprintf (file, " group:%d --> ??\n", group->id);
}
+ bool any_invariant = false;
for (i = 1; i <= data->max_inv_id; i++)
if (ivs->n_
Re: [PATCH 3/3] Enhance dumps of IVOPTS
On Fri, May 13, 2016 at 3:44 AM, Martin Liška wrote:
> On 05/13/2016 11:43 AM, Bin.Cheng wrote:
>> On Thu, May 12, 2016 at 5:41 PM, Martin Liška wrote:
>>> On 05/12/2016 03:51 PM, Bin.Cheng wrote:
On Thu, May 12, 2016 at 1:13 PM, Martin Liška wrote:
> On 05/10/2016 03:16 PM, Bin.Cheng wrote:
>> Another way is to remove the use of id for struct iv_inv_expr_ent once
>> for all. We can change iv_ca.used_inv_expr and cost_pair.inv_expr_id
>> to pointers, and rename iv_inv_expr_ent.id to count and use this to
>> record reference number in iv_ca. This if-statement on dump_file can
>> be saved. Also I think it simplifies current code a bit. For now,
>> there are id <-> struct maps for different structures in IVOPT which
>> make it not straightforward.
>
> Hi.
>
> I'm sending second version of the patch. I tried to follow your advices,
> but
> because of a iv_inv_expr_ent can simultaneously belong to multiply iv_cas,
> putting counter to iv_inv_expr_ent does not works. Instead of that, I've
> decided to replace used_inv_expr with a hash_map that contains used
> inv_exps
> and where value of the map is # of usages.
>
> Further questions:
> + iv_inv_expr_ent::id can be now removed as it's used just for purpose of
> dumps
> Group 0:
> cand costscaled freqcompl. depends on
> 5 2 2.001.000
> 6 4 4.001.001inv_expr:0
> 7 4 4.001.001inv_expr:1
> 8 4 4.001.001inv_expr:2
>
> That can be replaced with print_generic_expr, but I think using ids makes
> the dump
> output more clear.
I am okay with keeping id. Could you please dump all inv_exprs in a
single section like
:
inv_expr 0: print_generic_expr
inv_expr 1: ...
Then only dump the id afterwards?
>>>
>>> Sure, it would be definitely better:
>>>
>>> The new dump format looks:
>>>
>>> :
>>> inv_expr 0: sudoku_351(D) + (sizetype) S.833_774 * 4
>>> inv_expr 1: sudoku_351(D) + ((sizetype) S.833_774 * 4 +
>>> 18446744073709551580)
>>> inv_expr 2: sudoku_351(D) + ((sizetype) S.833_774 + 72) * 4
>>> inv_expr 3: sudoku_351(D) + ((sizetype) S.833_774 + 81) * 4
>>> inv_expr 4: &A.832 + (sizetype) _377 * 4
>>> inv_expr 5: &A.832 + ((sizetype) _377 * 4 + 18446744073709551612)
>>> inv_expr 6: &A.832 + ((sizetype) _377 + 8) * 4
>>> inv_expr 7: &A.832 + ((sizetype) _377 + 9) * 4
>>>
>>> :
>>> Group 0:
>>> cand costscaled freqcompl. depends on
>>>
>>> ...
>>>
>>> Improved to:
>>> cost: 27 (complexity 2)
>>> cand_cost: 11
>>> cand_group_cost: 10 (complexity 2)
>>> candidates: 3, 5
>>>group:0 --> iv_cand:5, cost=(2,0)
>>>group:1 --> iv_cand:5, cost=(4,1)
>>>group:2 --> iv_cand:5, cost=(4,1)
>>>group:3 --> iv_cand:3, cost=(0,0)
>>>group:4 --> iv_cand:3, cost=(0,0)
>>> invariants 1, 6
>>> invariant expressions 6, 3
>>>
>>> The only question here is that as used_inv_exprs are stored in a hash_map,
>>> order of dumped invariants would not be stable. Is it problem?
>> It is okay.
>>
>> Only nitpicking on this version.
>>
>>>
>
> + As check_GNU_style.sh reported multiple 8 spaces issues in hunks I've
> touched, I decided
> to fix all 8 spaces issues. Hope it's fine.
>
> I'm going to test the patch.
> Thoughts?
Some comments on the patch embedded.
>
> +/* Forward declaration. */
Not necessary.
> +struct iv_inv_expr_ent;
> +
>>>
>>> I think it's needed because struct cost_pair uses a pointer to
>>> iv_inv_expr_ent.
>> I mean the comment, clearly the declaration is self-documented.
>
> Hi.
>
> Yeah, removed.
>
>>
>>> @@ -6000,11 +6045,12 @@ iv_ca_set_no_cp (struct ivopts_data *data, struct
>>> iv_ca *ivs,
>>>
>>>iv_ca_set_remove_invariants (ivs, cp->depends_on);
>>>
>>> - if (cp->inv_expr_id != -1)
>>> + if (cp->inv_expr != NULL)
>>> {
>>> - ivs->used_inv_expr[cp->inv_expr_id]--;
>>> - if (ivs->used_inv_expr[cp->inv_expr_id] == 0)
>>> -ivs->num_used_inv_expr--;
>>> + unsigned *slot = ivs->used_inv_exprs->get (cp->inv_expr);
>>> + --(*slot);
>>> + if (*slot == 0)
>>> +ivs->used_inv_exprs->remove (cp->inv_expr);
>> I suppose insertion/removal of hash_map are not expensive? Because
>> the algorithm causes a lot of these operations.
>
> I think it should be ~ a constant operation.
>
>>
>>> @@ -6324,12 +6368,26 @@ iv_ca_dump (struct ivopts_data *data, FILE *file,
>>> struct iv_ca *ivs)
>>> fprintf (file, " group:%d --> ??\n", group->id);
>>> }
>>>
>>> + bool any_invariant = false;
>>>for (i = 1; i <= data->max_inv_id; i++)
>>> if (ivs->n_invariant_uses[i])
>>>{
>>> +const char *pref = any_invariant ? ", " : " invariants ";
>>> +any_invariant = true;
>>> fprintf (file, "%s%d", pref, i);
>>> -pref =
Re: [PATCH 3/3] Enhance dumps of IVOPTS
On 05/13/2016 11:43 AM, Bin.Cheng wrote:
> On Thu, May 12, 2016 at 5:41 PM, Martin Liška wrote:
>> On 05/12/2016 03:51 PM, Bin.Cheng wrote:
>>> On Thu, May 12, 2016 at 1:13 PM, Martin Liška wrote:
On 05/10/2016 03:16 PM, Bin.Cheng wrote:
> Another way is to remove the use of id for struct iv_inv_expr_ent once
> for all. We can change iv_ca.used_inv_expr and cost_pair.inv_expr_id
> to pointers, and rename iv_inv_expr_ent.id to count and use this to
> record reference number in iv_ca. This if-statement on dump_file can
> be saved. Also I think it simplifies current code a bit. For now,
> there are id <-> struct maps for different structures in IVOPT which
> make it not straightforward.
Hi.
I'm sending second version of the patch. I tried to follow your advices,
but
because of a iv_inv_expr_ent can simultaneously belong to multiply iv_cas,
putting counter to iv_inv_expr_ent does not works. Instead of that, I've
decided to replace used_inv_expr with a hash_map that contains used
inv_exps
and where value of the map is # of usages.
Further questions:
+ iv_inv_expr_ent::id can be now removed as it's used just for purpose of
dumps
Group 0:
cand costscaled freqcompl. depends on
5 2 2.001.000
6 4 4.001.001inv_expr:0
7 4 4.001.001inv_expr:1
8 4 4.001.001inv_expr:2
That can be replaced with print_generic_expr, but I think using ids makes
the dump
output more clear.
>>> I am okay with keeping id. Could you please dump all inv_exprs in a
>>> single section like
>>> :
>>> inv_expr 0: print_generic_expr
>>> inv_expr 1: ...
>>>
>>> Then only dump the id afterwards?
>>>
>>
>> Sure, it would be definitely better:
>>
>> The new dump format looks:
>>
>> :
>> inv_expr 0: sudoku_351(D) + (sizetype) S.833_774 * 4
>> inv_expr 1: sudoku_351(D) + ((sizetype) S.833_774 * 4 +
>> 18446744073709551580)
>> inv_expr 2: sudoku_351(D) + ((sizetype) S.833_774 + 72) * 4
>> inv_expr 3: sudoku_351(D) + ((sizetype) S.833_774 + 81) * 4
>> inv_expr 4: &A.832 + (sizetype) _377 * 4
>> inv_expr 5: &A.832 + ((sizetype) _377 * 4 + 18446744073709551612)
>> inv_expr 6: &A.832 + ((sizetype) _377 + 8) * 4
>> inv_expr 7: &A.832 + ((sizetype) _377 + 9) * 4
>>
>> :
>> Group 0:
>> cand costscaled freqcompl. depends on
>>
>> ...
>>
>> Improved to:
>> cost: 27 (complexity 2)
>> cand_cost: 11
>> cand_group_cost: 10 (complexity 2)
>> candidates: 3, 5
>>group:0 --> iv_cand:5, cost=(2,0)
>>group:1 --> iv_cand:5, cost=(4,1)
>>group:2 --> iv_cand:5, cost=(4,1)
>>group:3 --> iv_cand:3, cost=(0,0)
>>group:4 --> iv_cand:3, cost=(0,0)
>> invariants 1, 6
>> invariant expressions 6, 3
>>
>> The only question here is that as used_inv_exprs are stored in a hash_map,
>> order of dumped invariants would not be stable. Is it problem?
> It is okay.
>
> Only nitpicking on this version.
>
>>
+ As check_GNU_style.sh reported multiple 8 spaces issues in hunks I've
touched, I decided
to fix all 8 spaces issues. Hope it's fine.
I'm going to test the patch.
Thoughts?
>>>
>>> Some comments on the patch embedded.
>>>
+/* Forward declaration. */
>>> Not necessary.
+struct iv_inv_expr_ent;
+
>>
>> I think it's needed because struct cost_pair uses a pointer to
>> iv_inv_expr_ent.
> I mean the comment, clearly the declaration is self-documented.
Hi.
Yeah, removed.
>
>> @@ -6000,11 +6045,12 @@ iv_ca_set_no_cp (struct ivopts_data *data, struct
>> iv_ca *ivs,
>>
>>iv_ca_set_remove_invariants (ivs, cp->depends_on);
>>
>> - if (cp->inv_expr_id != -1)
>> + if (cp->inv_expr != NULL)
>> {
>> - ivs->used_inv_expr[cp->inv_expr_id]--;
>> - if (ivs->used_inv_expr[cp->inv_expr_id] == 0)
>> -ivs->num_used_inv_expr--;
>> + unsigned *slot = ivs->used_inv_exprs->get (cp->inv_expr);
>> + --(*slot);
>> + if (*slot == 0)
>> +ivs->used_inv_exprs->remove (cp->inv_expr);
> I suppose insertion/removal of hash_map are not expensive? Because
> the algorithm causes a lot of these operations.
I think it should be ~ a constant operation.
>
>> @@ -6324,12 +6368,26 @@ iv_ca_dump (struct ivopts_data *data, FILE *file,
>> struct iv_ca *ivs)
>> fprintf (file, " group:%d --> ??\n", group->id);
>> }
>>
>> + bool any_invariant = false;
>>for (i = 1; i <= data->max_inv_id; i++)
>> if (ivs->n_invariant_uses[i])
>>{
>> +const char *pref = any_invariant ? ", " : " invariants ";
>> +any_invariant = true;
>> fprintf (file, "%s%d", pref, i);
>> -pref = ", ";
>>}
>> +
>> + if (any_invariant)
>> +fprintf (file, "\n");
>> +
> To make dump easier to read, we can simply dump invariant
> variables/expressions unconditionally. Also keep inva
Re: [PATCH 3/3] Enhance dumps of IVOPTS
On Thu, May 12, 2016 at 5:41 PM, Martin Liška wrote:
> On 05/12/2016 03:51 PM, Bin.Cheng wrote:
>> On Thu, May 12, 2016 at 1:13 PM, Martin Liška wrote:
>>> On 05/10/2016 03:16 PM, Bin.Cheng wrote:
Another way is to remove the use of id for struct iv_inv_expr_ent once
for all. We can change iv_ca.used_inv_expr and cost_pair.inv_expr_id
to pointers, and rename iv_inv_expr_ent.id to count and use this to
record reference number in iv_ca. This if-statement on dump_file can
be saved. Also I think it simplifies current code a bit. For now,
there are id <-> struct maps for different structures in IVOPT which
make it not straightforward.
>>>
>>> Hi.
>>>
>>> I'm sending second version of the patch. I tried to follow your advices, but
>>> because of a iv_inv_expr_ent can simultaneously belong to multiply iv_cas,
>>> putting counter to iv_inv_expr_ent does not works. Instead of that, I've
>>> decided to replace used_inv_expr with a hash_map that contains used inv_exps
>>> and where value of the map is # of usages.
>>>
>>> Further questions:
>>> + iv_inv_expr_ent::id can be now removed as it's used just for purpose of
>>> dumps
>>> Group 0:
>>> cand costscaled freqcompl. depends on
>>> 5 2 2.001.000
>>> 6 4 4.001.001inv_expr:0
>>> 7 4 4.001.001inv_expr:1
>>> 8 4 4.001.001inv_expr:2
>>>
>>> That can be replaced with print_generic_expr, but I think using ids makes
>>> the dump
>>> output more clear.
>> I am okay with keeping id. Could you please dump all inv_exprs in a
>> single section like
>> :
>> inv_expr 0: print_generic_expr
>> inv_expr 1: ...
>>
>> Then only dump the id afterwards?
>>
>
> Sure, it would be definitely better:
>
> The new dump format looks:
>
> :
> inv_expr 0: sudoku_351(D) + (sizetype) S.833_774 * 4
> inv_expr 1: sudoku_351(D) + ((sizetype) S.833_774 * 4 +
> 18446744073709551580)
> inv_expr 2: sudoku_351(D) + ((sizetype) S.833_774 + 72) * 4
> inv_expr 3: sudoku_351(D) + ((sizetype) S.833_774 + 81) * 4
> inv_expr 4: &A.832 + (sizetype) _377 * 4
> inv_expr 5: &A.832 + ((sizetype) _377 * 4 + 18446744073709551612)
> inv_expr 6: &A.832 + ((sizetype) _377 + 8) * 4
> inv_expr 7: &A.832 + ((sizetype) _377 + 9) * 4
>
> :
> Group 0:
> cand costscaled freqcompl. depends on
>
> ...
>
> Improved to:
> cost: 27 (complexity 2)
> cand_cost: 11
> cand_group_cost: 10 (complexity 2)
> candidates: 3, 5
>group:0 --> iv_cand:5, cost=(2,0)
>group:1 --> iv_cand:5, cost=(4,1)
>group:2 --> iv_cand:5, cost=(4,1)
>group:3 --> iv_cand:3, cost=(0,0)
>group:4 --> iv_cand:3, cost=(0,0)
> invariants 1, 6
> invariant expressions 6, 3
>
> The only question here is that as used_inv_exprs are stored in a hash_map,
> order of dumped invariants would not be stable. Is it problem?
It is okay.
Only nitpicking on this version.
>
>>>
>>> + As check_GNU_style.sh reported multiple 8 spaces issues in hunks I've
>>> touched, I decided
>>> to fix all 8 spaces issues. Hope it's fine.
>>>
>>> I'm going to test the patch.
>>> Thoughts?
>>
>> Some comments on the patch embedded.
>>
>>>
>>> +/* Forward declaration. */
>> Not necessary.
>>> +struct iv_inv_expr_ent;
>>> +
>
> I think it's needed because struct cost_pair uses a pointer to
> iv_inv_expr_ent.
I mean the comment, clearly the declaration is self-documented.
> @@ -6000,11 +6045,12 @@ iv_ca_set_no_cp (struct ivopts_data *data, struct
> iv_ca *ivs,
>
>iv_ca_set_remove_invariants (ivs, cp->depends_on);
>
> - if (cp->inv_expr_id != -1)
> + if (cp->inv_expr != NULL)
> {
> - ivs->used_inv_expr[cp->inv_expr_id]--;
> - if (ivs->used_inv_expr[cp->inv_expr_id] == 0)
> -ivs->num_used_inv_expr--;
> + unsigned *slot = ivs->used_inv_exprs->get (cp->inv_expr);
> + --(*slot);
> + if (*slot == 0)
> +ivs->used_inv_exprs->remove (cp->inv_expr);
I suppose insertion/removal of hash_map are not expensive? Because
the algorithm causes a lot of these operations.
> @@ -6324,12 +6368,26 @@ iv_ca_dump (struct ivopts_data *data, FILE *file,
> struct iv_ca *ivs)
> fprintf (file, " group:%d --> ??\n", group->id);
> }
>
> + bool any_invariant = false;
>for (i = 1; i <= data->max_inv_id; i++)
> if (ivs->n_invariant_uses[i])
>{
> +const char *pref = any_invariant ? ", " : " invariants ";
> +any_invariant = true;
> fprintf (file, "%s%d", pref, i);
> -pref = ", ";
>}
> +
> + if (any_invariant)
> +fprintf (file, "\n");
> +
To make dump easier to read, we can simply dump invariant
variables/expressions unconditionally. Also keep invariant variables
and expressions in the same form.
const char *pref = "";
//...
fprintf (file, " invariant variables: "
for (i = 1; i <= data->max_inv_id; i++)
if (ivs->n_invariant_uses[i])
{
fprintf (file, "%s%d", pref, i);
pref = ", ";
Re: [PATCH 3/3] Enhance dumps of IVOPTS
On 05/12/2016 03:51 PM, Bin.Cheng wrote:
> On Thu, May 12, 2016 at 1:13 PM, Martin Liška wrote:
>> On 05/10/2016 03:16 PM, Bin.Cheng wrote:
>>> Another way is to remove the use of id for struct iv_inv_expr_ent once
>>> for all. We can change iv_ca.used_inv_expr and cost_pair.inv_expr_id
>>> to pointers, and rename iv_inv_expr_ent.id to count and use this to
>>> record reference number in iv_ca. This if-statement on dump_file can
>>> be saved. Also I think it simplifies current code a bit. For now,
>>> there are id <-> struct maps for different structures in IVOPT which
>>> make it not straightforward.
>>
>> Hi.
>>
>> I'm sending second version of the patch. I tried to follow your advices, but
>> because of a iv_inv_expr_ent can simultaneously belong to multiply iv_cas,
>> putting counter to iv_inv_expr_ent does not works. Instead of that, I've
>> decided to replace used_inv_expr with a hash_map that contains used inv_exps
>> and where value of the map is # of usages.
>>
>> Further questions:
>> + iv_inv_expr_ent::id can be now removed as it's used just for purpose of
>> dumps
>> Group 0:
>> cand costscaled freqcompl. depends on
>> 5 2 2.001.000
>> 6 4 4.001.001inv_expr:0
>> 7 4 4.001.001inv_expr:1
>> 8 4 4.001.001inv_expr:2
>>
>> That can be replaced with print_generic_expr, but I think using ids makes
>> the dump
>> output more clear.
> I am okay with keeping id. Could you please dump all inv_exprs in a
> single section like
> :
> inv_expr 0: print_generic_expr
> inv_expr 1: ...
>
> Then only dump the id afterwards?
>
Sure, it would be definitely better:
The new dump format looks:
:
inv_expr 0: sudoku_351(D) + (sizetype) S.833_774 * 4
inv_expr 1: sudoku_351(D) + ((sizetype) S.833_774 * 4 +
18446744073709551580)
inv_expr 2: sudoku_351(D) + ((sizetype) S.833_774 + 72) * 4
inv_expr 3: sudoku_351(D) + ((sizetype) S.833_774 + 81) * 4
inv_expr 4: &A.832 + (sizetype) _377 * 4
inv_expr 5: &A.832 + ((sizetype) _377 * 4 + 18446744073709551612)
inv_expr 6: &A.832 + ((sizetype) _377 + 8) * 4
inv_expr 7: &A.832 + ((sizetype) _377 + 9) * 4
:
Group 0:
cand costscaled freqcompl. depends on
...
Improved to:
cost: 27 (complexity 2)
cand_cost: 11
cand_group_cost: 10 (complexity 2)
candidates: 3, 5
group:0 --> iv_cand:5, cost=(2,0)
group:1 --> iv_cand:5, cost=(4,1)
group:2 --> iv_cand:5, cost=(4,1)
group:3 --> iv_cand:3, cost=(0,0)
group:4 --> iv_cand:3, cost=(0,0)
invariants 1, 6
invariant expressions 6, 3
The only question here is that as used_inv_exprs are stored in a hash_map,
order of dumped invariants would not be stable. Is it problem?
>>
>> + As check_GNU_style.sh reported multiple 8 spaces issues in hunks I've
>> touched, I decided
>> to fix all 8 spaces issues. Hope it's fine.
>>
>> I'm going to test the patch.
>> Thoughts?
>
> Some comments on the patch embedded.
>
>>
>> +/* Forward declaration. */
> Not necessary.
>> +struct iv_inv_expr_ent;
>> +
I think it's needed because struct cost_pair uses a pointer to iv_inv_expr_ent.
>
>>
>> /* Stores EXPR in DATA->inv_expr_tab, and assigns it an inv_expr_id. */
>>
>> -static int
>> +static iv_inv_expr_ent *
>> get_expr_id (struct ivopts_data *data, tree expr)
> We are not returning id any more, maybe rename to record_inv_expr or else.
Done.
>
>> {
>>struct iv_inv_expr_ent ent;
>> @@ -4806,13 +4809,13 @@ get_expr_id (struct ivopts_data *data, tree expr)
>>ent.hash = iterative_hash_expr (expr, 0);
>>slot = data->inv_expr_tab->find_slot (&ent, INSERT);
>>if (*slot)
>> -return (*slot)->id;
>> +return *slot;
>>
>>*slot = XNEW (struct iv_inv_expr_ent);
>>(*slot)->expr = expr;
>>(*slot)->hash = ent.hash;
>>(*slot)->id = data->max_inv_expr_id++;
>> - return (*slot)->id;
>> + return *slot;
> This could be changed to
> if (!*slot)
> {
> //new and insert
> }
> return *slot;
Also done.
>> }
>>
>> /* Returns the pseudo expr id if expression UBASE - RATIO * CBASE
>> @@ -4820,10 +4823,10 @@ get_expr_id (struct ivopts_data *data, tree expr)
>> ADDRESS_P is a flag indicating if the expression is for address
>> computation. */
>>
>> -static int
>> +static iv_inv_expr_ent *
>> get_loop_invariant_expr_id (struct ivopts_data *data, tree ubase,
>> -tree cbase, HOST_WIDE_INT ratio,
>> -bool address_p)
>> +tree cbase, HOST_WIDE_INT ratio,
>> +bool address_p)
> Rename function name here too.
>> {
>
Likewise.
>> @@ -5988,9 +5992,9 @@ determine_group_iv_costs (struct ivopts_data *data)
>>if (group->cost_map[j].depends_on)
>> bitmap_print (dump_file,
>>group->cost_map[j].depends_on, "","");
>> - if (group->cost_map[j].inv_expr_id != -1)
>> +if (group->cost_map[j].inv_expr != NULL)
>>
Re: [PATCH 3/3] Enhance dumps of IVOPTS
On Thu, May 12, 2016 at 1:13 PM, Martin Liška wrote:
> On 05/10/2016 03:16 PM, Bin.Cheng wrote:
>> Another way is to remove the use of id for struct iv_inv_expr_ent once
>> for all. We can change iv_ca.used_inv_expr and cost_pair.inv_expr_id
>> to pointers, and rename iv_inv_expr_ent.id to count and use this to
>> record reference number in iv_ca. This if-statement on dump_file can
>> be saved. Also I think it simplifies current code a bit. For now,
>> there are id <-> struct maps for different structures in IVOPT which
>> make it not straightforward.
>
> Hi.
>
> I'm sending second version of the patch. I tried to follow your advices, but
> because of a iv_inv_expr_ent can simultaneously belong to multiply iv_cas,
> putting counter to iv_inv_expr_ent does not works. Instead of that, I've
> decided to replace used_inv_expr with a hash_map that contains used inv_exps
> and where value of the map is # of usages.
>
> Further questions:
> + iv_inv_expr_ent::id can be now removed as it's used just for purpose of
> dumps
> Group 0:
> cand costscaled freqcompl. depends on
> 5 2 2.001.000
> 6 4 4.001.001inv_expr:0
> 7 4 4.001.001inv_expr:1
> 8 4 4.001.001inv_expr:2
>
> That can be replaced with print_generic_expr, but I think using ids makes the
> dump
> output more clear.
I am okay with keeping id. Could you please dump all inv_exprs in a
single section like
:
inv_expr 0: print_generic_expr
inv_expr 1: ...
Then only dump the id afterwards?
>
> + As check_GNU_style.sh reported multiple 8 spaces issues in hunks I've
> touched, I decided
> to fix all 8 spaces issues. Hope it's fine.
>
> I'm going to test the patch.
> Thoughts?
Some comments on the patch embedded.
>
> +/* Forward declaration. */
Not necessary.
> +struct iv_inv_expr_ent;
> +
>
> /* Stores EXPR in DATA->inv_expr_tab, and assigns it an inv_expr_id. */
>
> -static int
> +static iv_inv_expr_ent *
> get_expr_id (struct ivopts_data *data, tree expr)
We are not returning id any more, maybe rename to record_inv_expr or else.
> {
>struct iv_inv_expr_ent ent;
> @@ -4806,13 +4809,13 @@ get_expr_id (struct ivopts_data *data, tree expr)
>ent.hash = iterative_hash_expr (expr, 0);
>slot = data->inv_expr_tab->find_slot (&ent, INSERT);
>if (*slot)
> -return (*slot)->id;
> +return *slot;
>
>*slot = XNEW (struct iv_inv_expr_ent);
>(*slot)->expr = expr;
>(*slot)->hash = ent.hash;
>(*slot)->id = data->max_inv_expr_id++;
> - return (*slot)->id;
> + return *slot;
This could be changed to
if (!*slot)
{
//new and insert
}
return *slot;
> }
>
> /* Returns the pseudo expr id if expression UBASE - RATIO * CBASE
> @@ -4820,10 +4823,10 @@ get_expr_id (struct ivopts_data *data, tree expr)
> ADDRESS_P is a flag indicating if the expression is for address
> computation. */
>
> -static int
> +static iv_inv_expr_ent *
> get_loop_invariant_expr_id (struct ivopts_data *data, tree ubase,
> -tree cbase, HOST_WIDE_INT ratio,
> -bool address_p)
> +tree cbase, HOST_WIDE_INT ratio,
> +bool address_p)
Rename function name here too.
> {
> @@ -5988,9 +5992,9 @@ determine_group_iv_costs (struct ivopts_data *data)
>if (group->cost_map[j].depends_on)
> bitmap_print (dump_file,
>group->cost_map[j].depends_on, "","");
> - if (group->cost_map[j].inv_expr_id != -1)
> +if (group->cost_map[j].inv_expr != NULL)
> fprintf (dump_file, " inv_expr:%d",
> - group->cost_map[j].inv_expr_id);
> + group->cost_map[j].inv_expr->id);
Dump inv_expr in another column thus it won't appear under depends_on
in dump. Also make it preceding depends_on which is a bitmap.
While we are on this one before the other two, could you please make
this independent so it can be committed after rework?
Thanks,
bin
>
> Martin
Re: [PATCH 3/3] Enhance dumps of IVOPTS
On 05/10/2016 03:16 PM, Bin.Cheng wrote:
> Another way is to remove the use of id for struct iv_inv_expr_ent once
> for all. We can change iv_ca.used_inv_expr and cost_pair.inv_expr_id
> to pointers, and rename iv_inv_expr_ent.id to count and use this to
> record reference number in iv_ca. This if-statement on dump_file can
> be saved. Also I think it simplifies current code a bit. For now,
> there are id <-> struct maps for different structures in IVOPT which
> make it not straightforward.
Hi.
I'm sending second version of the patch. I tried to follow your advices, but
because of a iv_inv_expr_ent can simultaneously belong to multiply iv_cas,
putting counter to iv_inv_expr_ent does not works. Instead of that, I've
decided to replace used_inv_expr with a hash_map that contains used inv_exps
and where value of the map is # of usages.
Further questions:
+ iv_inv_expr_ent::id can be now removed as it's used just for purpose of dumps
Group 0:
cand costscaled freqcompl. depends on
5 2 2.001.000
6 4 4.001.001inv_expr:0
7 4 4.001.001inv_expr:1
8 4 4.001.001inv_expr:2
That can be replaced with print_generic_expr, but I think using ids makes the
dump
output more clear.
+ As check_GNU_style.sh reported multiple 8 spaces issues in hunks I've
touched, I decided
to fix all 8 spaces issues. Hope it's fine.
I'm going to test the patch.
Thoughts?
Martin
>From ce02c80c053c2a8a63ce6e87f5779a8dc5f470ee Mon Sep 17 00:00:00 2001
From: marxin
Date: Mon, 25 Apr 2016 14:29:01 +0200
Subject: [PATCH 3/3] Enhance dumps of IVOPTS
gcc/ChangeLog:
2016-05-12 Martin Liska
* tree-ssa-loop-ivopts.c (avg_loop_niter): Fix coding style.
(struct cost_pair): Replace inv_expr_id with direct pointer
to a iv_inv_expr_ent.
(struct iv_inv_expr_ent): Add comment for struct fields.
(struct iv_ca): Remove used_inv_exprs and replace it with a
hash_map called used_inv_exprs.
(niter_for_exit): Fix coding style.
(determine_base_object): Likewise.
(alloc_iv): Likewise.
(find_interesting_uses_outside): Likewise.
(add_candidate_1): Likewise.
(add_standard_iv_candidates): Likewise.
(set_group_iv_cost): Use inv_expr instead of inv_expr_id.
(prepare_decl_rtl): Fix coding style.
(get_address_cost): Likewise.
(get_shiftadd_cost): Likewise.
(force_expr_to_var_cost): Likewise.
(compare_aff_trees): Likewise.
(get_expr_id): Return iv_inv_expr_ent * instead of inv_expr_id.
(get_loop_invariant_expr_id): Likewise.
(get_computation_cost_at):
(get_computation_cost): Replace usage of inv_expr_id (int) with
inv_expr (iv_inv_expr_ent *).
(determine_group_iv_cost_generic): Likewise.
(determine_group_iv_cost_address): Likewise.
(iv_period): Fix coding style.
(iv_elimination_compare_lt): Likewise.
(may_eliminate_iv): Likewise.
(determine_group_iv_cost_cond): Replace usage of inv_expr_id (int) with
inv_expr (iv_inv_expr_ent *).
(determine_group_iv_costs): Likewise.
(iv_ca_recount_cost): Use used_inv_exprs to determine # of
used invariant expressions.
(iv_ca_set_remove_invariants): Fix coding style.
(iv_ca_set_no_cp): Use newly added hash_map.
(iv_ca_set_add_invariants): Likewise.
(iv_ca_set_cp): Likewise.
(iv_ca_new): Initialize the newly added hash_map.
(iv_ca_free): Delete it.
(iv_ca_dump): Fix coding style and dump used invariant
expressions.
(iv_ca_extend): Fix coding style.
(try_add_cand_for): Likewise.
(create_new_ivs): Display information about # of avg niters and
# of used invariant expressions.
(rewrite_use_compare): Fix coding style.
gcc/ChangeLog:
gcc/testsuite/ChangeLog:
2016-04-29 Martin Liska
* g++.dg/tree-ssa/ivopts-3.C: Change test-case to follow
the new format of dump output.
---
gcc/testsuite/g++.dg/tree-ssa/ivopts-3.C | 2 +-
gcc/tree-ssa-loop-ivopts.c | 378 ---
2 files changed, 201 insertions(+), 179 deletions(-)
diff --git a/gcc/testsuite/g++.dg/tree-ssa/ivopts-3.C b/gcc/testsuite/g++.dg/tree-ssa/ivopts-3.C
index 6194e9d..eb72581 100644
--- a/gcc/testsuite/g++.dg/tree-ssa/ivopts-3.C
+++ b/gcc/testsuite/g++.dg/tree-ssa/ivopts-3.C
@@ -72,4 +72,4 @@ int main ( int , char** ) {
// Verify that on x86_64 and i?86 we use a single IV for the innermost loop
-// { dg-final { scan-tree-dump "Selected IV set for loop \[0-9\]* at \[^ \]*:64, 1 IVs" "ivopts" { target x86_64-*-* i?86-*-* } } }
+// { dg-final { scan-tree-dump "Selected IV set for loop \[0-9\]* at \[^ \]*:64, 3 avg niters, 1 expressions, 1 IVs" "ivopts" { target x86_64-*-* i?86-*-* } } }
diff --git a/gcc/tree-ssa-loop-ivopts.c b/gcc/tree-ssa-loop-ivopts.c
index 17af590..5a48db2 100644
--- a/gcc/tree-ssa-loop-ivopts.c
+++ b/gcc/tree-ssa-loop-ivopts.c
@@ -130,7 +130,7 @@ avg_loop_niter (struct loop *loop)
{
niter = max_stmt_executions_int (loop);
if (niter == -1 || niter > AVG_LOOP_NITER (loop))
-return AVG_LOOP_NITER (loop);
+ return AVG_LOOP_NITER (loop);
}
Re: [PATCH 3/3] Enhance dumps of IVOPTS
On 05/10/2016 03:16 PM, Bin.Cheng wrote: > Another way is to remove the use of id for struct iv_inv_expr_ent once > for all. We can change iv_ca.used_inv_expr and cost_pair.inv_expr_id > to pointers, and rename iv_inv_expr_ent.id to count and use this to > record reference number in iv_ca. This if-statement on dump_file can > be saved. Also I think it simplifies current code a bit. For now, > there are id <-> struct maps for different structures in IVOPT which > make it not straightforward. Sound good to me, I will re-implement dump enhancement in suggested manner. Martin
Re: [PATCH 3/3] Enhance dumps of IVOPTS
On Mon, May 9, 2016 at 10:46 AM, Richard Biener wrote: > On Fri, May 6, 2016 at 11:19 AM, Martin Liška wrote: >> Hi. >> >> Honza asked me to explain the change more verbosely. >> The patch simplify enhances verbose dump of IVOPTS so that >> # of iterations is printed. Apart from that it also prints >> invariant expression that are used during the algorithm which >> considers a set of candidates which is improved. >> >> Main motivation for doing this was that sometimes the optimization >> considers a constant integer as invariant expression (Bin Cheng >> is working on removal of these) and that both IVs and IE are considered >> by the cost model to occupy a register. Which is not ideal and >> it sometimes tend to introduce more IVs that one would expect. >> >> === New format ===: >> Improved to: >> cost: 27 (complexity 2) >> cand_cost: 11 >> cand_group_cost: 10 (complexity 2) >> candidates: 3, 5 >>group:0 --> iv_cand:5, cost=(2,0) >>group:1 --> iv_cand:5, cost=(4,1) >>group:2 --> iv_cand:5, cost=(4,1) >>group:3 --> iv_cand:3, cost=(0,0) >>group:4 --> iv_cand:3, cost=(0,0) >> invariants 1, 6 >> used invariant expressions: >>inv_expr:3: ((sizetype) _976 - (sizetype) _922) * 4 >>inv_expr:6: ((sizetype) _1335 - (sizetype) _922) * 4 >> >> >> Original cost 27 (complexity 2) >> >> Final cost 27 (complexity 2) >> >> Selected IV set for loop 96 at original.f90:820, 5 avg niters, 2 >> expressions, 2 IVs: >> >> === Before ===: >> >> Improved to: >> cost: 27 (complexity 2) >> cand_cost: 11 >> cand_group_cost: 10 (complexity 2) >> candidates: 3, 5 >>group:0 --> iv_cand:5, cost=(2,0) >>group:1 --> iv_cand:5, cost=(4,1) >>group:2 --> iv_cand:5, cost=(4,1) >>group:3 --> iv_cand:3, cost=(0,0) >>group:4 --> iv_cand:3, cost=(0,0) >> invariants 1, 6 >> >> Original cost 27 (complexity 2) >> >> Final cost 27 (complexity 2) >> >> Selected IV set for loop 96 at original.f90:820, 2 IVs: > > But it slows donw compile-time just for enhanced dump files. Can you > make the new > hash-map conditional on dumping? Hi, Another way is to remove the use of id for struct iv_inv_expr_ent once for all. We can change iv_ca.used_inv_expr and cost_pair.inv_expr_id to pointers, and rename iv_inv_expr_ent.id to count and use this to record reference number in iv_ca. This if-statement on dump_file can be saved. Also I think it simplifies current code a bit. For now, there are id <-> struct maps for different structures in IVOPT which make it not straightforward. Thanks, bin > > Richard. > >> >> Martin
Re: [PATCH 3/3] Enhance dumps of IVOPTS
On Fri, May 6, 2016 at 11:19 AM, Martin Liška wrote: > Hi. > > Honza asked me to explain the change more verbosely. > The patch simplify enhances verbose dump of IVOPTS so that > # of iterations is printed. Apart from that it also prints > invariant expression that are used during the algorithm which > considers a set of candidates which is improved. > > Main motivation for doing this was that sometimes the optimization > considers a constant integer as invariant expression (Bin Cheng > is working on removal of these) and that both IVs and IE are considered > by the cost model to occupy a register. Which is not ideal and > it sometimes tend to introduce more IVs that one would expect. > > === New format ===: > Improved to: > cost: 27 (complexity 2) > cand_cost: 11 > cand_group_cost: 10 (complexity 2) > candidates: 3, 5 >group:0 --> iv_cand:5, cost=(2,0) >group:1 --> iv_cand:5, cost=(4,1) >group:2 --> iv_cand:5, cost=(4,1) >group:3 --> iv_cand:3, cost=(0,0) >group:4 --> iv_cand:3, cost=(0,0) > invariants 1, 6 > used invariant expressions: >inv_expr:3: ((sizetype) _976 - (sizetype) _922) * 4 >inv_expr:6: ((sizetype) _1335 - (sizetype) _922) * 4 > > > Original cost 27 (complexity 2) > > Final cost 27 (complexity 2) > > Selected IV set for loop 96 at original.f90:820, 5 avg niters, 2 expressions, > 2 IVs: > > === Before ===: > > Improved to: > cost: 27 (complexity 2) > cand_cost: 11 > cand_group_cost: 10 (complexity 2) > candidates: 3, 5 >group:0 --> iv_cand:5, cost=(2,0) >group:1 --> iv_cand:5, cost=(4,1) >group:2 --> iv_cand:5, cost=(4,1) >group:3 --> iv_cand:3, cost=(0,0) >group:4 --> iv_cand:3, cost=(0,0) > invariants 1, 6 > > Original cost 27 (complexity 2) > > Final cost 27 (complexity 2) > > Selected IV set for loop 96 at original.f90:820, 2 IVs: But it slows donw compile-time just for enhanced dump files. Can you make the new hash-map conditional on dumping? Richard. > > Martin
Re: [PATCH 3/3] Enhance dumps of IVOPTS
Hi. Honza asked me to explain the change more verbosely. The patch simplify enhances verbose dump of IVOPTS so that # of iterations is printed. Apart from that it also prints invariant expression that are used during the algorithm which considers a set of candidates which is improved. Main motivation for doing this was that sometimes the optimization considers a constant integer as invariant expression (Bin Cheng is working on removal of these) and that both IVs and IE are considered by the cost model to occupy a register. Which is not ideal and it sometimes tend to introduce more IVs that one would expect. === New format ===: Improved to: cost: 27 (complexity 2) cand_cost: 11 cand_group_cost: 10 (complexity 2) candidates: 3, 5 group:0 --> iv_cand:5, cost=(2,0) group:1 --> iv_cand:5, cost=(4,1) group:2 --> iv_cand:5, cost=(4,1) group:3 --> iv_cand:3, cost=(0,0) group:4 --> iv_cand:3, cost=(0,0) invariants 1, 6 used invariant expressions: inv_expr:3: ((sizetype) _976 - (sizetype) _922) * 4 inv_expr:6: ((sizetype) _1335 - (sizetype) _922) * 4 Original cost 27 (complexity 2) Final cost 27 (complexity 2) Selected IV set for loop 96 at original.f90:820, 5 avg niters, 2 expressions, 2 IVs: === Before ===: Improved to: cost: 27 (complexity 2) cand_cost: 11 cand_group_cost: 10 (complexity 2) candidates: 3, 5 group:0 --> iv_cand:5, cost=(2,0) group:1 --> iv_cand:5, cost=(4,1) group:2 --> iv_cand:5, cost=(4,1) group:3 --> iv_cand:3, cost=(0,0) group:4 --> iv_cand:3, cost=(0,0) invariants 1, 6 Original cost 27 (complexity 2) Final cost 27 (complexity 2) Selected IV set for loop 96 at original.f90:820, 2 IVs: Martin
