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

[Sebastian Pop]

Hi Tobi,

attached is your patch updated with all my comments, and a second patch that
implements the innermost parallel loop detection.

Thanks,
Sebastian

Sebastian Pop wrote:
> Hi Tobi,
> 
> Thanks for working on this.  I have a few comments inline.
> 
> Tobias Grosser wrote:
> > +// Information about an ast node.
> > +struct AstNodeUserInfo {
> > +  // The node is openmp parallel.
> > +  int IsOpenmp;
> 
> I don't like the name IsOpenmp because we are referencing OpenMP: we can very
> well generate code calling POSIX threads and that's not a good reason to call it
> IsPosix either.
> 
> I was thinking about calling it IsParallel, but after thinking about it, I don't
> think this is a good name either: we are tagging the outermost parallel loop.
> What about IsOutermostParallel?  For the SIMD vectorization we would tag the
> loops with IsInnermostParallel.
> 
> > +static __isl_give isl_printer *printForOpenmp(__isl_keep isl_ast_node *Node,
> > +        __isl_take isl_printer *Printer,
> > +        __isl_take isl_ast_print_options *PrintOptions) {
> 
> indent
> 
> > +  Printer = isl_printer_start_line(Printer);
> > +  Printer = isl_printer_print_str(Printer, "#pragma omp parallel for");
> 
> I also don't like that we are generating openmp pragmas in the pretty printer.
> As I don't have an alternative for this, I am fine with keeping this notation.
> 
> > +static __isl_give isl_printer *printFor(__isl_take isl_printer *Printer,
> > +        __isl_take isl_ast_print_options *PrintOptions,
> > +        __isl_keep isl_ast_node *Node, void *User) {
> 
> indent
> 
> > +  isl_id *Id;
> > +  int Openmp;
> > +
> > +  Openmp = 0;
> > +  Id = isl_ast_node_get_annotation(Node);
> > +
> > +  if (Id) {
> > +    struct AstNodeUserInfo *Info;
> > +
> > +    Info = (struct AstNodeUserInfo *) isl_id_get_user(Id);
> > +    if (Info && Info->IsOpenmp)
> > +      Openmp = 1;
> > +  }
> > +
> > +  if (Openmp)
> > +    Printer = printForOpenmp(Node, Printer, PrintOptions);
> > +  else
> > +    Printer = isl_ast_node_for_print(Node, Printer, PrintOptions);
> > +
> > +  isl_id_free(Id);
> 
> You may free an Id that is NULL here: see the "if (Id)" above?
> 
> > +static __isl_give isl_ast_node *astBuildAfterFor(__isl_take isl_ast_node *Node,
> > +        __isl_keep isl_ast_build *Build, void *User) {
> 
> indent
> 
> >  static __isl_give isl_ast_node *AtEachDomain(__isl_keep isl_ast_node *Node,
> > -       __isl_keep isl_ast_build *Context, void *User)
> > +	__isl_keep isl_ast_build *Context, void *User)
> 
> indent
> 
> Thanks,
> Sebastian
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-- 
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: 0001-isl-Detect-openmp-parallelism.patch
Type: text/x-diff
Size: 19949 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121213/d1bc797b/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-isl-detect-vector-parallelism.patch
Type: text/x-diff
Size: 5795 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121213/d1bc797b/attachment-0001.patch>