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

[Sebastian Pop]

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