Re: [PATCH v4] Use range-based for loops for traversing loops

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Thomas,

on 2021/7/30 下午3:18, Thomas Schwinge wrote:
> Hi!
> 
> Thanks for this nice clean-up.
> 
> Curious why in some instances we're not removing the 'class loop *loop'
> declaration, I had a look, and this may suggest some further clean-up?
> (See below; so far, all untested!)
> 
> 

Yeah, since this patch is mainly to replace FOR_EACH_LOOP* for loop
traserval, I didn't scan all the class loop *loop, just those ones
used by FOR_EACH_LOOP*.  I like your nice proposed further clean-up,
thanks for doing that!

> But first, is this transformation correct?
> 
>> --- a/gcc/tree-cfg.c
>> +++ b/gcc/tree-cfg.c
> 
>> @@ -7752,9 +7747,9 @@ move_sese_region_to_fn (struct function *dest_cfun, basic_block entry_bb,
>>
>>    /* Fix up orig_loop_num.  If the block referenced in it has been moved
>>       to dest_cfun, update orig_loop_num field, otherwise clear it.  */
>> -  class loop *dloop;
>> +  class loop *dloop = NULL;
>>    signed char *moved_orig_loop_num = NULL;
>> -  FOR_EACH_LOOP_FN (dest_cfun, dloop, 0)
>> +  for (class loop *dloop : loops_list (dest_cfun, 0))
>>      if (dloop->orig_loop_num)
>>        {
>>       if (moved_orig_loop_num == NULL)
> 
> We've got the original outer 'dloop' and a new separate inner 'dloop'
> inside the 'for'.  The outer one now is only ever 'NULL'-initialized --
> but then meant to be used in later code (not shown here)?  (I cannot
> claim to understand this later code usage of 'dloop', though.  Maybe even
> is there a pre-existing problem here?  Or, it's still too early on Friday
> morning.)  If there is an actual problem, would the following restore the
> original behavior?

Good question, I also noticed this before.  I think it's a pre-existing
problem since the loop iterating will terminate till dloop is NULL as
there are none early breaks.  So IMHO the initialization with NULL should
be the same as before.  The following path using dloop very likely doesn't
got executed, I planed to dig into the associated test case with graphite
enabled later.  Anyway, thanks for raising this!

BR,
Kewen

> 
>     --- gcc/tree-cfg.c
>     +++ gcc/tree-cfg.c
>     @@ -7747,9 +7747,9 @@ move_sese_region_to_fn (struct function *dest_cfun, basic_block entry_bb,
> 
>        /* Fix up orig_loop_num.  If the block referenced in it has been moved
>           to dest_cfun, update orig_loop_num field, otherwise clear it.  */
>     -  class loop *dloop = NULL;
>     +  class loop *dloop;
>        signed char *moved_orig_loop_num = NULL;
>     -  for (class loop *dloop : loops_list (dest_cfun, 0))
>     +  for (dloop : loops_list (dest_cfun, 0))
>          if (dloop->orig_loop_num)
>            {
>         if (moved_orig_loop_num == NULL)
> 
> 
> Second, additional clean-up possible as follows?
> 
>> --- a/gcc/cfgloop.c
>> +++ b/gcc/cfgloop.c
> 
>> @@ -1457,7 +1452,7 @@ verify_loop_structure (void)
>>    auto_sbitmap visited (last_basic_block_for_fn (cfun));
>>    bitmap_clear (visited);
>>    bbs = XNEWVEC (basic_block, n_basic_blocks_for_fn (cfun));
>> -  FOR_EACH_LOOP (loop, LI_FROM_INNERMOST)
>> +  for (auto loop : loops_list (cfun, LI_FROM_INNERMOST))
>>      {
>>        unsigned n;
>>
>> @@ -1503,7 +1498,7 @@ verify_loop_structure (void)
>>    free (bbs);
>>
>>    /* Check headers and latches.  */
>> -  FOR_EACH_LOOP (loop, 0)
>> +  for (auto loop : loops_list (cfun, 0))
>>      {
>>        i = loop->num;
>>        if (loop->header == NULL)
>> @@ -1629,7 +1624,7 @@ verify_loop_structure (void)
>>      }
>>
>>    /* Check the recorded loop exits.  */
>> -  FOR_EACH_LOOP (loop, 0)
>> +  for (auto loop : loops_list (cfun, 0))
>>      {
>>        if (!loop->exits || loop->exits->e != NULL)
>>       {
>> @@ -1723,7 +1718,7 @@ verify_loop_structure (void)
>>         err = 1;
>>       }
>>
>> -      FOR_EACH_LOOP (loop, 0)
>> +      for (auto loop : loops_list (cfun, 0))
>>       {
>>         eloops = 0;
>>         for (exit = loop->exits->next; exit->e; exit = exit->next)
> 
>     --- gcc/cfgloop.c
>     +++ gcc/cfgloop.c
>     @@ -1398,7 +1398,6 @@ verify_loop_structure (void)
>      {
>        unsigned *sizes, i, j;
>        basic_block bb, *bbs;
>     -  class loop *loop;
>        int err = 0;
>        edge e;
>        unsigned num = number_of_loops (cfun);
>     @@ -1690,7 +1689,7 @@ verify_loop_structure (void)
>               for (; exit; exit = exit->next_e)
>                 eloops++;
> 
>     -         for (loop = bb->loop_father;
>     +         for (class loop *loop = bb->loop_father;
>                    loop != e->dest->loop_father
>                    /* When a loop exit is also an entry edge which
>                       can happen when avoiding CFG manipulations
> 
>> --- a/gcc/ipa-fnsummary.c
>> +++ b/gcc/ipa-fnsummary.c
>> @@ -2923,7 +2923,7 @@ analyze_function_body (struct cgraph_node *node, bool early)
>>        if (dump_file && (dump_flags & TDF_DETAILS))
>>       flow_loops_dump (dump_file, NULL, 0);
>>        scev_initialize ();
>> -      FOR_EACH_LOOP (loop, 0)
>> +      for (auto loop : loops_list (cfun, 0))
>>       {
>>         predicate loop_iterations = true;
>>         sreal header_freq;
> 
>     --- gcc/ipa-fnsummary.c
>     +++ gcc/ipa-fnsummary.c
>     @@ -2916,7 +2916,6 @@ analyze_function_body (struct cgraph_node *node, bool early)
>        if (nonconstant_names.exists () && !early)
>          {
>            ipa_fn_summary *s = ipa_fn_summaries->get (node);
>     -      class loop *loop;
>            unsigned max_loop_predicates = opt_for_fn (node->decl,
>                                                  param_ipa_max_loop_predicates);
> 
>     @@ -2960,7 +2959,7 @@ analyze_function_body (struct cgraph_node *node, bool early)
>            /* To avoid quadratic behavior we analyze stride predicates only
>               with respect to the containing loop.  Thus we simply iterate
>          over all defs in the outermost loop body.  */
>     -      for (loop = loops_for_fn (cfun)->tree_root->inner;
>     +      for (class loop *loop = loops_for_fn (cfun)->tree_root->inner;
>            loop != NULL; loop = loop->next)
>         {
>           predicate loop_stride = true;
> 
>> --- a/gcc/loop-init.c
>> +++ b/gcc/loop-init.c
> 
>> @@ -229,7 +228,7 @@ fix_loop_structure (bitmap changed_bbs)
>>       loops, so that when we remove the loops, we know that the loops inside
>>       are preserved, and do not waste time relinking loops that will be
>>       removed later.  */
>> -  FOR_EACH_LOOP (loop, LI_FROM_INNERMOST)
>> +  for (auto loop : loops_list (cfun, LI_FROM_INNERMOST))
>>      {
>>        /* Detect the case that the loop is no longer present even though
>>           it wasn't marked for removal.
> 
>     --- gcc/loop-init.c
>     +++ gcc/loop-init.c
>     @@ -201,7 +201,6 @@ fix_loop_structure (bitmap changed_bbs)
>      {
>        basic_block bb;
>        int record_exits = 0;
>     -  class loop *loop;
>        unsigned old_nloops, i;
> 
>        timevar_push (TV_LOOP_INIT);
>     @@ -279,6 +278,7 @@ fix_loop_structure (bitmap changed_bbs)
> 
>        /* Finally free deleted loops.  */
>        bool any_deleted = false;
>     +  class loop *loop;
>        FOR_EACH_VEC_ELT (*get_loops (cfun), i, loop)
>          if (loop && loop->header == NULL)
>            {
> 
>> --- a/gcc/loop-invariant.c
>> +++ b/gcc/loop-invariant.c
>> @@ -2136,7 +2136,7 @@ calculate_loop_reg_pressure (void)
>>    rtx link;
>>    class loop *loop, *parent;
>>
>> -  FOR_EACH_LOOP (loop, 0)
>> +  for (auto loop : loops_list (cfun, 0))
>>      if (loop->aux == NULL)
>>        {
>>       loop->aux = xcalloc (1, sizeof (class loop_data));
>> @@ -2203,7 +2203,7 @@ calculate_loop_reg_pressure (void)
>>    bitmap_release (&curr_regs_live);
>>    if (flag_ira_region == IRA_REGION_MIXED
>>        || flag_ira_region == IRA_REGION_ALL)
>> -    FOR_EACH_LOOP (loop, 0)
>> +    for (auto loop : loops_list (cfun, 0))
>>        {
>>       EXECUTE_IF_SET_IN_BITMAP (&LOOP_DATA (loop)->regs_live, 0, j, bi)
>>         if (! bitmap_bit_p (&LOOP_DATA (loop)->regs_ref, j))
>> @@ -2217,7 +2217,7 @@ calculate_loop_reg_pressure (void)
>>        }
>>    if (dump_file == NULL)
>>      return;
>> -  FOR_EACH_LOOP (loop, 0)
>> +  for (auto loop : loops_list (cfun, 0))
>>      {
>>        parent = loop_outer (loop);
>>        fprintf (dump_file, "\n  Loop %d (parent %d, header bb%d, depth %d)\n",
> 
>     --- gcc/loop-invariant.c
>     +++ gcc/loop-invariant.c
>     @@ -2134,7 +2134,7 @@ calculate_loop_reg_pressure (void)
>        basic_block bb;
>        rtx_insn *insn;
>        rtx link;
>     -  class loop *loop, *parent;
>     +  class loop *parent;
> 
>        for (auto loop : loops_list (cfun, 0))
>          if (loop->aux == NULL)
>     @@ -2151,7 +2151,7 @@ calculate_loop_reg_pressure (void)
>            if (curr_loop == current_loops->tree_root)
>         continue;
> 
>     -      for (loop = curr_loop;
>     +      for (class loop *loop = curr_loop;
>            loop != current_loops->tree_root;
>            loop = loop_outer (loop))
>         bitmap_ior_into (&LOOP_DATA (loop)->regs_live, DF_LR_IN (bb));
> 
>> --- a/gcc/predict.c
>> +++ b/gcc/predict.c
>> @@ -1949,7 +1949,7 @@ predict_loops (void)
>>
>>    /* Try to predict out blocks in a loop that are not part of a
>>       natural loop.  */
>> -  FOR_EACH_LOOP (loop, LI_FROM_INNERMOST)
>> +  for (auto loop : loops_list (cfun, LI_FROM_INNERMOST))
>>      {
>>        basic_block bb, *bbs;
>>        unsigned j, n_exits = 0;
> 
>     --- gcc/predict.c
>     +++ gcc/predict.c
>     @@ -1927,7 +1927,6 @@ predict_extra_loop_exits (edge exit_edge)
>      static void
>      predict_loops (void)
>      {
>     -  class loop *loop;
>        basic_block bb;
>        hash_set <class loop *> with_recursion(10);
> 
>     @@ -1941,7 +1941,7 @@ predict_loops (void)
>             && (decl = gimple_call_fndecl (gsi_stmt (gsi))) != NULL
>             && recursive_call_p (current_function_decl, decl))
>           {
>     -       loop = bb->loop_father;
>     +       class loop *loop = bb->loop_father;
>             while (loop && !with_recursion.add (loop))
>               loop = loop_outer (loop);
>           }
> 
>> --- a/gcc/tree-loop-distribution.c
>> +++ b/gcc/tree-loop-distribution.c
>> @@ -3312,7 +3312,7 @@ loop_distribution::execute (function *fun)
>>
>>    /* We can at the moment only distribute non-nested loops, thus restrict
>>       walking to innermost loops.  */
>> -  FOR_EACH_LOOP (loop, LI_ONLY_INNERMOST)
>> +  for (auto loop : loops_list (cfun, LI_ONLY_INNERMOST))
>>      {
>>        /* Don't distribute multiple exit edges loop, or cold loop when
>>           not doing pattern detection.  */
> 
>     --- gcc/tree-loop-distribution.c
>     +++ gcc/tree-loop-distribution.c
>     @@ -3293,7 +3293,6 @@ prepare_perfect_loop_nest (class loop *loop)
>      unsigned int
>      loop_distribution::execute (function *fun)
>      {
>     -  class loop *loop;
>        bool changed = false;
>        basic_block bb;
>        control_dependences *cd = NULL;
>     @@ -3384,6 +3383,7 @@ loop_distribution::execute (function *fun)
>            /* Destroy loop bodies that could not be reused.  Do this late as we
>          otherwise can end up refering to stale data in control dependences.  */
>            unsigned i;
>     +      class loop *loop;
>            FOR_EACH_VEC_ELT (loops_to_be_destroyed, i, loop)
>         destroy_loop (loop);
> 
> 
>> --- a/gcc/tree-vectorizer.c
>> +++ b/gcc/tree-vectorizer.c
>> @@ -1194,7 +1194,7 @@ vectorize_loops (void)
>>    /* If some loop was duplicated, it gets bigger number
>>       than all previously defined loops.  This fact allows us to run
>>       only over initial loops skipping newly generated ones.  */
>> -  FOR_EACH_LOOP (loop, 0)
>> +  for (auto loop : loops_list (cfun, 0))
>>      if (loop->dont_vectorize)
>>        {
>>       any_ifcvt_loops = true;
>> @@ -1213,7 +1213,7 @@ vectorize_loops (void)
>>                 loop4 (copy of loop2)
>>               else
>>                 loop5 (copy of loop4)
>> -        If FOR_EACH_LOOP gives us loop3 first (which has
>> +        If loops' iteration gives us loop3 first (which has
>>          dont_vectorize set), make sure to process loop1 before loop4;
>>          so that we can prevent vectorization of loop4 if loop1
>>          is successfully vectorized.  */
> 
>     --- gcc/tree-vectorizer.c
>     +++ gcc/tree-vectorizer.c
>     @@ -1172,7 +1172,6 @@ vectorize_loops (void)
>        unsigned int i;
>        unsigned int num_vectorized_loops = 0;
>        unsigned int vect_loops_num;
>     -  class loop *loop;
>        hash_table<simduid_to_vf> *simduid_to_vf_htab = NULL;
>        hash_table<simd_array_to_simduid> *simd_array_to_simduid_htab = NULL;
>        bool any_ifcvt_loops = false;
>     @@ -1256,7 +1255,7 @@ vectorize_loops (void)
>        if (any_ifcvt_loops)
>          for (i = 1; i < number_of_loops (cfun); i++)
>            {
>     -   loop = get_loop (cfun, i);
>     +   class loop *loop = get_loop (cfun, i);
>         if (loop && loop->dont_vectorize)
>           {
>             gimple *g = vect_loop_vectorized_call (loop);
>     @@ -1282,7 +1281,7 @@ vectorize_loops (void)
>            loop_vec_info loop_vinfo;
>            bool has_mask_store;
> 
>     -      loop = get_loop (cfun, i);
>     +      class loop *loop = get_loop (cfun, i);
>            if (!loop || !loop->aux)
>         continue;
>            loop_vinfo = (loop_vec_info) loop->aux;
> 
> 
> Grüße
>  Thomas
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
>