On Tue, Nov 7, 2017 at 10:53 AM, Richard Biener
<richard.guent...@gmail.com> wrote:
> On Fri, Nov 3, 2017 at 1:40 PM, Bin Cheng <bin.ch...@arm.com> wrote:
>> Hi,
>> As described in message of previous patch:
>>
>> This patch set fixes both PRs in the opposite way: Instead of find dominance
>> insertion position for root reference, we resort zero-distance references of
>> combined chain by their position information so that new root reference must
>> dominate others. This should be more efficient because we avoid function
>> call
>> to stmt_dominates_stmt_p.
>> Bootstrap and test on x86_64 and AArch64 in patch set. Is it OK?
>
> +/* { dg-additional-options "-mavx2" { target avx2_runtime } } */
>
> you don't need avx2_runtime for -mavx2 so please instead use
> { target { x86_64-*-* i?86-*-* } }
>
> +#include <map>
> +#define INCLUDE_ALGORITHM /* std::sort */
>
> can you please use GCCs own hash_map? Btw...
>
> + /* Setup UID for all statements in dominance order. */
> + std::map<gimple *, int> stmts_map;
> + basic_block *bbs = get_loop_body_in_dom_order (loop);
> + for (i = 0; i < loop->num_nodes; i++)
> + {
> + int uid = 0;
> + basic_block bb = bbs[i];
> +
> + for (gimple_stmt_iterator bsi = gsi_start_phis (bb); !gsi_end_p (bsi);
> + gsi_next (&bsi))
> + {
> + gimple *stmt = gsi_stmt (bsi);
> + if (!virtual_operand_p (gimple_phi_result (as_a<gphi *> (stmt))))
> + stmts_map[stmt] = uid;
>
> why don't you use gimple_[set_]uid ()? Given you do a dominance check
> you don't even need to do this in dominance order - usually passes just
> number UIDs in all relevant BBs. There is a helper for that as well,
> renumber_gimple_stmt_uids_in_blocks which can be used on
> the get_loop_body result.
Yea, I forgot gimple_[set_]uid interface when doing this. All fixed now.
>
> + /* Sort all ZERO distance references by position. */
> + std::sort (&ch1->refs[0], &ch1->refs[0] + j, order_drefs_by_pos);
> +
>
> given ch1->refs is a vec you can use the new vec::qsort_block you added
> instead of including algorithm and using std::sort.
Sorry, I haven't push that patch in. In this updated patch, I fall
back to generic qsort so algorithm is not included.
Bootstrap and test on x86_64. Is it OK?
Thanks,
bin
2017-11-10 Bin Cheng <bin.ch...@arm.com>
PR tree-optimization/82726
PR tree-optimization/70754
* tree-predcom.c (order_drefs_by_pos): New function.
(combine_chains): Move code setting has_max_use_after to...
(try_combine_chains): ...here. New parameter. Sort combined chains
according to position information.
(tree_predictive_commoning_loop): Update call to above function.
(update_pos_for_combined_chains, pcom_stmt_dominates_stmt_p): New.
gcc/testsuite
2017-11-10 Bin Cheng <bin.ch...@arm.com>
PR tree-optimization/82726
* gcc.dg/tree-ssa/pr82726.c: New test.
>
> Richard.
>
>> Thanks,
>> bin
>> 2017-11-02 Bin Cheng <bin.ch...@arm.com>
>>
>> PR tree-optimization/82726
>> PR tree-optimization/70754
>> * tree-predcom.c (<map>, INCLUDE_ALGORITHM): New headers.
>> (order_drefs_by_pos): New function.
>> (combine_chains): Move code setting has_max_use_after to...
>> (try_combine_chains): ...here. New parameter. Sort combined chains
>> according to position information.
>> (tree_predictive_commoning_loop): Update call to above function.
>> (update_pos_for_combined_chains, pcom_stmt_dominates_stmt_p): New.
>>
>> gcc/testsuite
>> 2017-11-02 Bin Cheng <bin.ch...@arm.com>
>>
>> PR tree-optimization/82726
>> * gcc.dg/tree-ssa/pr82726.c: New test.
