On Mon, Dec 4, 2017 at 5:39 PM, Richard Biener <rguent...@suse.de> wrote:
> On December 4, 2017 5:01:45 PM GMT+01:00, "Bin.Cheng" <amker.ch...@gmail.com>
> wrote:
>>On Mon, Dec 4, 2017 at 3:43 PM, Richard Biener <rguent...@suse.de>
>>wrote:
>>>
>>> When skimming through the code I noticed the following (chatted on
>>IRC
>>> about parts of the changes).
>>>
>>> Bootstrap / regtest running on x86_64-unknown-linux-gnu.
>>>
>>> Will commit tomorrow unless you beat me to that.
>>>
>>> Richard.
>>>
>>> 2017-12-04 Richard Biener <rguent...@suse.de>
>>>
>>> * gimple-loop-interchange.cc
>>(loop_cand::classify_simple_reduction):
>>> Simplify.
>>> (loop_cand::analyze_iloop_reduction_var): Reject dead
>>reductions.
>>> (loop_cand::analyze_oloop_reduction_var): Likewise.
>>Simplify.
>>> (tree_loop_interchange::interchange_loops): Properly analyze
>>> scalar evolution before instantiating a SCEV.
>>>
>>> Index: gcc/gimple-loop-interchange.cc
>>> ===================================================================
>>> --- gcc/gimple-loop-interchange.cc (revision 255383)
>>> +++ gcc/gimple-loop-interchange.cc (working copy)
>>> @@ -444,50 +444,21 @@ loop_cand::classify_simple_reduction (re
>>> if (!bb || bb->loop_father != m_outer)
>>> return;
>>>
>>> - if (!is_gimple_assign (producer))
>>> + if (!gimple_assign_load_p (producer))
>>> return;
>>>
>>> - code = gimple_assign_rhs_code (producer);
>>> - if (get_gimple_rhs_class (code) != GIMPLE_SINGLE_RHS)
>>> - return;
>>> -
>>> - lhs = gimple_assign_lhs (producer);
>>> - if (lhs != re->init)
>>> - return;
>>> -
>>> - rhs = gimple_assign_rhs1 (producer);
>>> - if (!REFERENCE_CLASS_P (rhs))
>>> - return;
>>> -
>>> - re->init_ref = rhs;
>>> + re->init_ref = gimple_assign_rhs1 (producer);
>>> }
>>> else if (!CONSTANT_CLASS_P (re->init))
>>> return;
>>>
>>> - /* Check how reduction variable is used. Note usually reduction
>>variable
>>> - is used outside of its defining loop, we don't require that in
>>terms of
>>> - loop interchange. */
>>> - if (!re->lcssa_phi)
>>> - consumer = single_use_in_loop (re->next, m_loop);
>>> - else
>>> - consumer = single_use_in_loop (PHI_RESULT (re->lcssa_phi),
>>m_outer);
>>> -
>>> - if (!consumer || !is_gimple_assign (consumer))
>>> - return;
>>> -
>>> - code = gimple_assign_rhs_code (consumer);
>>> - if (get_gimple_rhs_class (code) != GIMPLE_SINGLE_RHS)
>>> - return;
>>> -
>>> - lhs = gimple_assign_lhs (consumer);
>>> - if (!REFERENCE_CLASS_P (lhs))
>>> - return;
>>> -
>>> - rhs = gimple_assign_rhs1 (consumer);
>>> - if (rhs != PHI_RESULT (re->lcssa_phi))
>>> + /* Check how reduction variable is used. */
>>> + consumer = single_use_in_loop (PHI_RESULT (re->lcssa_phi),
>>m_outer);
>>> + if (!consumer
>>> + || !gimple_store_p (consumer))
>>> return;
>>>
>>> - re->fini_ref = lhs;
>>> + re->fini_ref = gimple_get_lhs (consumer);
>>> re->consumer = consumer;
>>>
>>> /* Simple reduction with constant initializer. */
>>> @@ -608,6 +579,9 @@ loop_cand::analyze_iloop_reduction_var (
>>> else
>>> return false;
>>> }
>>> + if (!lcssa_phi)
>>> + return false;
>>> +
>>> re = XCNEW (struct reduction);
>>> re->var = var;
>>> re->init = init;
>>> @@ -681,15 +655,9 @@ loop_cand::analyze_oloop_reduction_var (
>>>
>>> /* Outer loop's reduction should only be used to initialize inner
>>loop's
>>> simple reduction. */
>>> - FOR_EACH_IMM_USE_FAST (use_p, iterator, var)
>>> - {
>>> - stmt = USE_STMT (use_p);
>>> - if (is_gimple_debug (stmt))
>>> - continue;
>>> -
>>> - if (stmt != inner_re->phi)
>>> - return false;
>>> - }
>>> + if (! single_imm_use (var, &use_p, &stmt)
>>> + || stmt != inner_re->phi)
>>> + return false;
>>>
>>> /* Check this reduction is correctly used outside of loop via
>>lcssa phi. */
>>> FOR_EACH_IMM_USE_FAST (use_p, iterator, next)
>>> @@ -711,6 +679,8 @@ loop_cand::analyze_oloop_reduction_var (
>>> else
>>> return false;
>>> }
>>> + if (!lcssa_phi)
>>> + return false;
>>>
>>> re = XCNEW (struct reduction);
>>> re->var = var;
>>> @@ -1146,12 +1116,18 @@ tree_loop_interchange::interchange_loops
>>> edge instantiate_below = loop_preheader_edge (loop_nest);
>>> gsi = gsi_last_bb (loop_preheader_edge (loop_nest)->src);
>>> i_niters = number_of_latch_executions (iloop.m_loop);
>>> - i_niters = instantiate_scev (instantiate_below, loop_nest,
>>i_niters);
>>> + i_niters = analyze_scalar_evolution (loop_outer (iloop.m_loop),
>>i_niters);
>>> + i_niters = instantiate_scev (instantiate_below, loop_outer
>>(iloop.m_loop),
>>> + i_niters);
>>> i_niters = force_gimple_operand_gsi (&gsi, unshare_expr
>>(i_niters), true,
>>> NULL_TREE, false,
>>GSI_CONTINUE_LINKING);
>>> o_niters = number_of_latch_executions (oloop.m_loop);
>>> if (oloop.m_loop != loop_nest)
>>> - o_niters = instantiate_scev (instantiate_below, loop_nest,
>>o_niters);
>>> + {
>>> + o_niters = analyze_scalar_evolution (loop_outer
>>(oloop.m_loop), o_niters);
>>> + o_niters = instantiate_scev (instantiate_below, loop_outer
>>(oloop.m_loop),
>>> + o_niters);
>>> + }
>>Hmm, sorry to disturb. IIRC, it's important to instantiate niters
>>against the outermost loop in nest. Otherwise niters could refer to
>>intermediate variables defined by loop in nest, which may lead to
>>undefined ssa use issue in the chain of interchange.
>
>
> That's a common misconception of the instantiate_scev interface. We
> instantiate against below, the loop argument is to interpret the chrec and
> absolutely has to match what loop you called analyze_evolution on. Otherwise
> there's a chance you get garbage out (from garbage in).
Ah, right. Thanks for explaining.
Thanks,
bin
>
> Richard.
>>Thanks,
>>bin
>>> o_niters = force_gimple_operand_gsi (&gsi, unshare_expr
>>(o_niters), true,
>>> NULL_TREE, false,
>>GSI_CONTINUE_LINKING);
>>>
>
