[llvm-commits] [PATCH][polly] isl: Detect openmp parallel loops
Tobias Grosser
tobias at grosser.es
Wed Dec 12 22:50:33 PST 2012
On 12/13/2012 07:05 AM, Sebastian Pop wrote:
> Hi Tobi,
>
> attached is your patch updated with all my comments, and a second patch that
> implements the innermost parallel loop detection.
Thanks, I committed my patch. Here some comments on yours.
> From cdf52944867c6ca384f578b989e56518fbe7684d Mon Sep 17 00:00:00 2001
> From: Sebastian Pop<spop at codeaurora.org>
> Date: Wed, 12 Dec 2012 23:49:05 -0600
> Subject: [PATCH 2/2] isl: detect vector parallelism
>
> ---
> lib/CodeGen/IslAst.cpp | 60 ++++++++++++++++----
> .../OpenMP/nested_loop_both_parallel_parametric.ll | 2 +-
> test/Isl/Ast/OpenMP/nested_loop_inner_parallel.ll | 2 +-
> test/Isl/Ast/OpenMP/single_loop_param_parallel.ll | 2 +-
> 4 files changed, 52 insertions(+), 14 deletions(-)
>
> diff --git a/lib/CodeGen/IslAst.cpp b/lib/CodeGen/IslAst.cpp
> index 4f79ea8..3481c00 100644
> --- a/lib/CodeGen/IslAst.cpp
> +++ b/lib/CodeGen/IslAst.cpp
> @@ -81,6 +81,9 @@ static void IslAstUserFree(void *User)
> struct AstNodeUserInfo {
> // The node is the outermost parallel loop.
> int IsOutermostParallel;
> +
> + // The node is the innermost parallel loop.
> + int IsInnermostParallel;
> };
>
> // Temporary information used when building the ast.
> @@ -92,14 +95,16 @@ struct AstBuildUserInfo {
> int InParallelFor;
> };
>
> -// Print a loop annotated with OpenMP pragmas.
> +// Print a loop annotated with OpenMP or vector pragmas.
> static __isl_give isl_printer *
> printParallelFor(__isl_keep isl_ast_node *Node, __isl_take isl_printer *Printer,
> __isl_take isl_ast_print_options *PrintOptions,
> AstNodeUserInfo *Info) {
> if (Info && Info->IsOutermostParallel) {
This line does not sound correct. You only print simd pragmas, if there
is an outermost parallel loop.
> Printer = isl_printer_start_line(Printer);
> - if (Info->IsOutermostParallel)
> + if (Info->IsInnermostParallel)
> + Printer = isl_printer_print_str(Printer, "#pragma vector for");
Could we use the established '#pragma simd'?
> + else if (Info->IsOutermostParallel)
> Printer = isl_printer_print_str(Printer, "#pragma omp parallel for");
> Printer = isl_printer_end_line(Printer);
Also, I don't think we should use an if/else here. If the loop is marked
both as innermost and outermost parallel, we should print both pragmas.
> }
> @@ -126,6 +131,7 @@ static struct AstNodeUserInfo *allocateAstNodeUserInfo() {
> struct AstNodeUserInfo *NodeInfo;
> NodeInfo = (struct AstNodeUserInfo *) malloc(sizeof(struct AstNodeUserInfo));
> NodeInfo->IsOutermostParallel = 0;
> + NodeInfo->IsInnermostParallel = 0;
> return NodeInfo;
> }
>
> @@ -223,6 +229,38 @@ static __isl_give isl_id *astBuildBeforeFor(__isl_keep isl_ast_build *Build,
> return Id;
> }
>
> +// Returns 0 when Node contains loops, otherwise returns -1.
> +static int hasLoops(__isl_take isl_ast_node *Node, void *User) {
You should use 'bool' here.
Could you call it containsLoops? Why are you passing around 'User'? As
far as I can see, it is never used.
> + if (!Node)
> + return -1;
> +
> + switch (isl_ast_node_get_type(Node)) {
> + case isl_ast_node_for:
> + isl_ast_node_free(Node);
> + return 0;
> + case isl_ast_node_block: {
> + isl_ast_node_list *List = isl_ast_node_block_get_children(Node);
> + int Res = isl_ast_node_list_foreach(List, &hasLoops, NULL);
> + isl_ast_node_list_free(List);
> + isl_ast_node_free(Node);
> + return Res;
> + }
> + case isl_ast_node_if: {
> + int Res = -1;
> + if (0 == hasLoops(isl_ast_node_if_get_then(Node), NULL) ||
Why do you compare to '0'. You should return a boolean in hasLoops
and just rely on its value.
Also, Instead of the '||' you may decide to just return if the first
part of the if-condition is false.
> + (isl_ast_node_if_has_else(Node) &&
> + 0 == hasLoops(isl_ast_node_if_get_else(Node), NULL)))
> + Res = 0;
> + isl_ast_node_free(Node);
> + return Res;
> + }
> + case isl_ast_node_user:
> + default:
> + isl_ast_node_free(Node);
> + return -1;
> + }
> +}
> +
> // This method is executed after the construction of a for node.
> //
> // It performs the following actions:
> @@ -233,17 +271,17 @@ static __isl_give isl_id *astBuildBeforeFor(__isl_keep isl_ast_build *Build,
> static __isl_give isl_ast_node *
> astBuildAfterFor(__isl_take isl_ast_node *Node,
> __isl_keep isl_ast_build *Build, void *User) {
> - isl_id *Id;
> - struct AstBuildUserInfo *BuildInfo;
> - struct AstNodeUserInfo *Info;
> -
> - Id = isl_ast_node_get_annotation(Node);
> + isl_id *Id = isl_ast_node_get_annotation(Node);
> if (!Id)
> return Node;
> - Info = (struct AstNodeUserInfo *) isl_id_get_user(Id);
> - if (Info && Info->IsOutermostParallel) {
> - BuildInfo = (struct AstBuildUserInfo *) User;
> - BuildInfo->InParallelFor = 0;
> + struct AstNodeUserInfo *Info = (struct AstNodeUserInfo *) isl_id_get_user(Id);
> + struct AstBuildUserInfo *BuildInfo = (struct AstBuildUserInfo *) User;
> + if (Info) {
> + if (Info->IsOutermostParallel)
> + BuildInfo->InParallelFor = 0;
> + if (0 != hasLoops(isl_ast_node_for_get_body(Node), NULL))
Why are you comparing agains '0'? This should be a boolean negation, no?
> + if (astScheduleDimIsParallel(Build, BuildInfo->Deps))
> + Info->IsInnermostParallel = 1;
> }
>
> isl_id_free(Id);
> diff --git a/test/Isl/Ast/OpenMP/nested_loop_both_parallel_parametric.ll b/test/Isl/Ast/OpenMP/nested_loop_both_parallel_parametric.ll
> index b87610a..1b31741 100644
> --- a/test/Isl/Ast/OpenMP/nested_loop_both_parallel_parametric.ll
> +++ b/test/Isl/Ast/OpenMP/nested_loop_both_parallel_parametric.ll
We probably want to rename the OpenMP directory to 'Parallelism'. Or we
just get rid of it and add a non parallel test case to it too.
> @@ -50,6 +50,6 @@ ret:
> ; memory accesses, that would happen if n >= 1024.
> ;
> ; CHECK: for (int c1 = 0; c1 < n; c1 += 1)
> -; CHECK: #pragma omp parallel for
> +; CHECK: #pragma vector for
> ; CHECK: for (int c3 = 0; c3 < n; c3 += 1)
> ; CHECK: Stmt_loop_body(c1, c3);
It may make sense to have another option. Or, if possible, to allow
optional parameters such as:
-polly-ast-detect-parallelism=Outermost|Innermost|Both|None
We could then avoid reducing the OpenMP test coverage. Instead we could
get the test outputs for each of these option values.
> diff --git a/test/Isl/Ast/OpenMP/nested_loop_inner_parallel.ll b/test/Isl/Ast/OpenMP/nested_loop_inner_parallel.ll
> index c530aaf..d16f59f 100644
> --- a/test/Isl/Ast/OpenMP/nested_loop_inner_parallel.ll
> +++ b/test/Isl/Ast/OpenMP/nested_loop_inner_parallel.ll
> @@ -41,6 +41,6 @@ ret:
> }
>
> ; CHECK: for (int c1 = 0; c1 < n; c1 += 1)
> -; CHECK: #pragma omp parallel for
> +; CHECK: #pragma vector for
> ; CHECK: for (int c3 = 0; c3 < n; c3 += 1)
> ; CHECK: Stmt_loop_body(c1, c3);
> diff --git a/test/Isl/Ast/OpenMP/single_loop_param_parallel.ll b/test/Isl/Ast/OpenMP/single_loop_param_parallel.ll
> index 499cfa7..f73e820 100644
> --- a/test/Isl/Ast/OpenMP/single_loop_param_parallel.ll
> +++ b/test/Isl/Ast/OpenMP/single_loop_param_parallel.ll
> @@ -30,6 +30,6 @@ ret:
> ret void
> }
>
> -; CHECK: #pragma omp parallel for
> +; CHECK: #pragma vector for
> ; CHECK: for (int c1 = 0; c1 < n; c1 += 1)
> ; CHECK: Stmt_loop_body(c1)
> -- 1.7.9.5
Otherwise the patch looks good. Please commit after if you agree with
the comments.
Tobi
More information about the llvm-commits
mailing list