Re: [PING][PR67476] Add param parloops-schedule

[Tom de Vries <Tom_deVries at mentor dot com>]

On 22/09/15 12:58, Bernd Schmidt wrote:
> On 09/22/2015 09:19 AM, Tom de Vries wrote:
> > These two patches:
> > - https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00938.html
> > - https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00940.html
> > add a param parloop-schedule=<static|dynamic|guided|auto|runtime>.
>
> The problem I have when trying to review them is that the second patch
> does quite a bit more, and there's no description of these changes in
> your mail or in the comments. Please explain what the non-obvious parts
> of the patch do.

Hi Bernd,

I'll try to give a bit of context:

The omp-expand machinery is used in two contexts:
1. when omp annotations are added to the source. In that case,
   omp-expand is used in non-ssa gimple context.
2. when parloops annotates a loop with omp annotations. In that case,
   omp-expand is used in ssa gimple context.

Until recently, parloops generated only
  #pragma omp for schedule (static)
annotations, which means the loop was expanded by 
expand_omp_for_static_nochunk, which has ssa support.

Recently (r227434), I've added --param parloops-chunk-size, which means 
parloops can also generate
  #pragma omp for schedule (static, chunk-size)
annotations, which means the loop is expanded by 
expand_omp_for_static_chunk. There was some ssa support in that function 
(I'm not sure when this was added), but it was incomplete and broken, so 
the patch series for the param contained patches to fix this (r227436 - 
r227438).

Similarly, this patch adds --params 
parloops-schedule<static|dynamic|guided|auto|runtime>, which means 
parloops can also generate
 #pragma omp for schedule (dynamic)
 #pragma omp for schedule (dynamic, chunk-size)
 #pragma omp for schedule (guided)
 #pragma omp for schedule (guided, chunk-size)
 #pragma omp for schedule (auto)
 #pragma omp for schedule (runtime)
annotations. While f.i. 'schedule (auto)' is mapped on 'schedule 
(static)' and uses expand_omp_for_static_nochunk, for f.i. 'schedule 
(runtime)' we use expand_omp_for_generic. Again, there was some ssa 
support in that function, but incomplete and broken, so this patch 
contains fixes for that.

In addition, I've recently (r226427) fixed parloops such that it no 
longer invalidates the loops structure and cancels the loop tree. At the 
parloops side, that involved adding an empty latch block, in order not 
to break the LOOPS_HAVE_SIMPLE_LATCHES property. At the omp-expand side, 
that meant handling the empty latch block, as well as updating the loops 
structure. In r227435, I've applied a similar fix for 
expand_omp_for_static_chunk.

In summary, since this patch means that expand_omp_for_generic is 
triggered in ssa mode by parloops, I needed to fix these 3 things in 
expand_omp_for_generic:
- fixing and completing ssa support
- handling added empty latch block
- updating loops structure

> For example - this thing is entirely unexplained:
> > +  if (gimple_in_ssa_p (cfun))
> > +    {
> > +      gphi_iterator psi;
> > +
> > +      for (psi = gsi_start_phis (l3_bb); !gsi_end_p (psi); gsi_next
> > (&psi))
> > +    {
> > +      source_location locus;
> > +      gphi *nphi;
> > +      gphi *exit_phi = psi.phi ();
> > +
> > +      edge l2_to_l3 = find_edge (l2_bb, l3_bb);
> > +      tree exit_res = PHI_ARG_DEF_FROM_EDGE (exit_phi, l2_to_l3);
> >
> > +      basic_block latch = BRANCH_EDGE (cont_bb)->dest;
> > +      edge latch_to_l1 = find_edge (latch, l1_bb);
> > +      gphi *inner_phi = find_phi_with_arg_on_edge (exit_res,
> > latch_to_l1);
> > +
> > +      tree t = gimple_phi_result (exit_phi);
> > +      tree new_res = copy_ssa_name (t, NULL);
> > +      nphi = create_phi_node (new_res, l0_bb);
> > +
> > +      edge l0_to_l1 = find_edge (l0_bb, l1_bb);
> > +      t = PHI_ARG_DEF_FROM_EDGE (inner_phi, l0_to_l1);
> > +      locus = gimple_phi_arg_location_from_edge (inner_phi, l0_to_l1);
> > +      edge entry_to_l0 = find_edge (entry_bb, l0_bb);
> > +      add_phi_arg (nphi, t, entry_to_l0, locus);
> > +
> > +      edge l2_to_l0 = find_edge (l2_bb, l0_bb);
> > +      add_phi_arg (nphi, exit_res, l2_to_l0, UNKNOWN_LOCATION);
> > +
> > +      add_phi_arg (inner_phi, new_res, l0_to_l1, UNKNOWN_LOCATION);
> > +    };
> > +    }
> > +

This bit is adding missing ssa support. In expand_omp_for_generic, we 
add a loop around the loop we're expanding. Since we're in ssa, that 
means we need to:
- add phis to the outer loop that connect to the phis in the inner,
  original loop, and
- move the loop entry value of the inner phi to the loop entry value of
  the outer phi.

> Also, it would be good to know why having this param is important. Is it
> a tool for development? Are users expected to use it?

It's a tool to try out diffent omp-schedules for parloops-generated 
loops. I'd expect users and developers with an interest in 
auto-parallelization to use it.

Thanks,
- Tom