[Polly][Refactor] IslAst [6/7]

[Tobias Grosser]

On 29/07/2014 20:18, Johannes Doerfert wrote:
>>> OK. Thanks for clarifying.
> :)
>
>>> >>I was assuming this, but I am still a little confused by the code (also
> by the old one). Specifically, I try to understand if there is a difference
> between an innermost-parallel loop and an innermost and parallel loop and
> wonder which has been implemented before, what is implemented today.
> There should not be a difference. I think (very strongly):
>                 Old
> New
>    InnermostParallel <= equiv. =>  innermost && parallel
>   OutermostParallel <= equiv. => !innermost && parallel

The semantics for an outermost parallel loop have changed at two places.
I give details inline.

It is fine to suggest semantic changes, but we definitely do not want to 
mix them with a refactoring change. Also, if you would like to make 
semantic changes, we need a motivation why the current approach is 
inappropriate and the new semantics are superior in some way.


> 0001-Refactor-Use-isParallel-and-isInnermost.patch
>
>
>  From 1c17cc6ce53ab9cbc865c46f21165f0548313fb3 Mon Sep 17 00:00:00 2001
> From: Johannes Doerfert<jdoerfert at codeaurora.org>
> Date: Wed, 16 Jul 2014 16:35:55 -0700
> Subject: [PATCH] [Refactor] Use isParallel and isInnermost
>
>    + Change isInnermostParallel and isOuterParallel to combinations of isParallel
>      and isInnermost.
>    + Perform the parallelism check on the innermost loop only once.
>    + Inline the markOpenmpParallel function.
>    + Rename all IslAstUserPayload * into Payload (was Info, NodeInfo and Payload
>      before).

I think in general a one or two line description of why you change 
something helps to understand why you believe something is beneficial.

> diff --git a/lib/CodeGen/IslAst.cpp b/lib/CodeGen/IslAst.cpp
> index 0910835..7d3e24c 100644
> --- a/lib/CodeGen/IslAst.cpp
> +++ b/lib/CodeGen/IslAst.cpp
> @@ -104,22 +104,20 @@ struct AstBuildUserInfo {
>   static __isl_give isl_printer *
>   printParallelFor(__isl_keep isl_ast_node *Node, __isl_take isl_printer *Printer,
>                    __isl_take isl_ast_print_options *PrintOptions,
> -                 IslAstUserPayload *Info) {
> -  if (Info) {
> -    if (Info->IsInnermostParallel) {
> +                 IslAstUserPayload *Payload) {
> +  if (Payload && Payload->IsParallel) {
> +    if (Payload->IsInnermost) {
>         Printer = isl_printer_start_line(Printer);
>         Printer = isl_printer_print_str(Printer, "#pragma simd");
> -      if (Info->IsReductionParallel)
> -        Printer = isl_printer_print_str(Printer, " reduction");
> -      Printer = isl_printer_end_line(Printer);
> -    }
> -    if (Info->IsOutermostParallel) {
> -      Printer = isl_printer_start_line(Printer);
> -      Printer = isl_printer_print_str(Printer, "#pragma omp parallel for");
> -      if (Info->IsReductionParallel)
> +      if (Payload->IsReductionParallel)
>           Printer = isl_printer_print_str(Printer, " reduction");
>         Printer = isl_printer_end_line(Printer);
>       }
> +    Printer = isl_printer_start_line(Printer);
> +    Printer = isl_printer_print_str(Printer, "#pragma omp parallel for");
> +    if (Payload->IsReductionParallel)
> +      Printer = isl_printer_print_str(Printer, " reduction");
> +    Printer = isl_printer_end_line(Printer);

The printing changed semantics. Before this change, we never nested 
pragma omp parallel for statements. Now we do. (See committed test case).

> -  markOpenmpParallel(Build, BuildInfo, NodeInfo);
> +  // Test for parallelism only if we are not already inside a parallel loop

Point at the end of the sentence.

> +  assert(!Payload->Build && "Build environment already set");
> +  Payload->Build = isl_ast_build_copy(Build);
> +
> +  // Test innermost loops even if we are in a parallel for but only if it
> +  // was not set because of the loop itself. Also, reset the "in parallel for
> +  // flag" if necessary.

This says exactly what you are doing, but I have a hard time to 
understand it. I think the point you want to make is that certain loops 
have not yet been tested for parallelism and that we want to test them 
in case they are innermost.

What about something like the following:

// Innermost loops that are surrounded by parallel loops have not yet
// been tested for parallelism. Test them here to ensure we check all
// innermost loops for parallelism.

> +  if (Payload->IsInnermost && !Payload->IsParallel && BuildInfo->InParallelFor)
> +    Payload->IsParallel = astScheduleDimIsParallel(
> +        Build, BuildInfo->Deps, Payload->IsReductionParallel);
> +  else if (Payload->IsParallel)
> +    BuildInfo->InParallelFor = false;
>
>     isl_id_free(Id);
>     return Node;


>   bool IslAstInfo::isInnermostParallel(__isl_keep isl_ast_node *Node) {
>     IslAstUserPayload *Payload = getNodePayload(Node);
> -  return Payload && Payload->IsInnermostParallel &&
> +  return Payload && Payload->IsInnermost && Payload->IsParallel &&
>            !Payload->IsReductionParallel;
>   }

Can we rename this function to isInnermostAndParallel to avoid confusion 
with a check for an innermost loop that is parallel.

>   bool IslAstInfo::isOuterParallel(__isl_keep isl_ast_node *Node) {
>     IslAstUserPayload *Payload = getNodePayload(Node);
> -  return Payload && Payload->IsOutermostParallel &&
> +  return Payload && !Payload->IsInnermost && Payload->IsParallel &&
>            !Payload->IsReductionParallel;
>   }

I believe the semantics of this function changed. In case we have a 
single parallel loop, this function would have returned 'true' before
and now returns 'false'. (When running -analyze we would still print
a #pragma omp parallel for statement, but I would prefer for the 
printing to use the same function which we can use in the code generator
to find the loop that we want to omp parallelize.)

Cheers,
Tobias