[gimple-interchange] Random cleanups

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.

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.

Thanks,
bin
>    o_niters = force_gimple_operand_gsi (&gsi, unshare_expr (o_niters), true,
>                                        NULL_TREE, false, GSI_CONTINUE_LINKING);
>

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). 

Richard. 
>Thanks,
>bin
>>    o_niters = force_gimple_operand_gsi (&gsi, unshare_expr
>(o_niters), true,
>>                                        NULL_TREE, false,
>GSI_CONTINUE_LINKING);
>>

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);
>>>
>