[llvm-commits] [PATCH][polly] isl: Detect openmp parallel loops
Sebastian Pop
spop at codeaurora.org
Wed Dec 12 23:15:32 PST 2012
Tobias Grosser wrote:
> 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.
I have not paid close enough attention. Thanks for pointing this out.
>
> > 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'?
sure
>
> >+ 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.
ok
>
> > }
> >@@ -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.
Nop, ISL documents the interface for isl_ast_node_list_foreach: it should be
exactly what I have here, returns 0 on success and -1 on fail.
> Could you call it containsLoops?
I will change this name.
> Why are you passing around 'User'?
> As far as I can see, it is never used.
>
I don't need User: ISL says it needs it: here is the relevant part of the manual
int isl_set_list_foreach(__isl_keep isl_set_list *list,
int (*fn)(__isl_take isl_set *el, void *user),
void *user);
> >+ 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.
No, because hasLoops is a recursive function relying on the ISL way to traverse
lists of isl_ast_nodes.
>
> Also, Instead of the '||' you may decide to just return if the first
> part of the if-condition is false.
>
And that would make the code longer, as I have to free Node before returning.
> >+ (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?
>
ISL says so.
> >+ 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
>
I prefer doing that in a separate patch.
> 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.
Thanks for the review. See attached the updated patch.
Sebastian
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-isl-detect-vector-parallelism.patch
Type: text/x-diff
Size: 6083 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121213/42640f80/attachment.patch>
More information about the llvm-commits
mailing list