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

[Sebastian Pop]

Tobias Grosser wrote:
> 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.

I think I also like the wrapper way as well.  I will also add the comment to
explain why the search function returns these unusual values.

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

I will keep the ||

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

Ok, let me address the wrapper issue and I will then commit the patch.

Thanks for your review,
Sebastian
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation