[llvm-commits] [PATCH][polly] isl: Detect openmp parallel loops

[Tobias Grosser]

On 12/13/2012 08:15 AM, Sebastian Pop wrote:
> Tobias Grosser wrote:
>> >On 12/13/2012 07:05 AM, Sebastian Pop wrote:
>> >
>>> > >    }
>>> > >@@ -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);

OK.

You may want to add a comment to the top of the containsLoops function 
to point this out. Alternatively, you can move the containsLoops() 
function to the C++ style interface and write a one-line wrapper 
function that can be used as callback.

>>> > >+  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.

I have no strong opinion about this. Go whatever way you prefer.

>>> > >@@ -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.

As the current patch does not reduce the OpenMP test coverage, this is 
fine with me.

Except the minor comments above, the attached patch looks good.
Tobi