Re: [PATCH][gimple-interchange] Random cleanups

["Bin.Cheng" <amker dot cheng at gmail dot com>]

On Mon, Dec 4, 2017 at 5:39 PM, Richard Biener <rguenther@suse.de> wrote:
> On December 4, 2017 5:01:45 PM GMT+01:00, "Bin.Cheng" <amker.cheng@gmail.com> wrote:
>>On Mon, Dec 4, 2017 at 3:43 PM, Richard Biener <rguenther@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  <rguenther@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);
>>>
>