[Polly][Refactor] IslAst [3/7]

[Johannes Doerfert]

r213795 fixes the "typos" introduced in r213792

--

Johannes Doerfert
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation


-----Original Message-----
From: Johannes Doerfert [mailto:jdoerfert at codeaurora.org] 
Sent: Wednesday, July 23, 2014 1:26 PM
To: 'Tobias Grosser'; 'llvm-commits at cs.uiuc.edu'
Subject: RE: [Polly][Refactor] IslAst [3/7]

Thanks for the quick review and comments!

r213792

--

Johannes Doerfert
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation


-----Original Message-----
From: Tobias Grosser [mailto:tobias at grosser.es]
Sent: Wednesday, July 23, 2014 12:12 PM
To: Johannes Doerfert; llvm-commits at cs.uiuc.edu
Subject: Re: [Polly][Refactor] IslAst [3/7]

On 23/07/2014 20:35, Johannes Doerfert wrote:
> Here is the next patch of the IslAst serious.
>

Nice patch. Some comments from my side.

Tobias
> --
>
> Johannes Doerfert
>
> Employee of Qualcomm Innovation Center, Inc.
>
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
> hosted by The Linux Foundation
>
>
> 0003-Refactor-IslAst-and-payload-struct.patch
>
>
>  From 5e02dd3cff7c8ce4f850f7094312991f947a1276 Mon Sep 17 00:00:00
> 2001
> From: Johannes Doerfert<jdoerfert at codeaurora.org>
> Date: Wed, 16 Jul 2014 15:28:38 -0700
> Subject: [PATCH] [Refactor] IslAst and payload struct
>
>    + Renamed context into build when it's the isl_ast_build
>    + Use the IslAstInfo functions to extract the schedule of a node
>    + Use the IslAstInfo functions to extract the build/context of a node
>    + Move the payload struct into the IslAstInfo class
>    + Use a constructor and destructor (also new and delete) to
>      allocate/initialize the payload struct
> ---
>   include/polly/CodeGen/IslAst.h    |  40 ++++++++++----
>   lib/CodeGen/IslAst.cpp            | 108
+++++++++++++++++++-------------------
>   lib/CodeGen/IslCodeGeneration.cpp |  24 +--------
>   3 files changed, 85 insertions(+), 87 deletions(-)
>
> diff --git a/include/polly/CodeGen/IslAst.h 
> b/include/polly/CodeGen/IslAst.h index ab10a54..c7a8992 100644
> --- a/include/polly/CodeGen/IslAst.h
> +++ b/include/polly/CodeGen/IslAst.h
> @@ -40,20 +40,32 @@ namespace polly {
>   class Scop;
>   class IslAst;
>
> -// Information about an ast node.
> -struct IslAstUserPayload {
> -  struct isl_ast_build *Context;
> -  // The node is the outermost parallel loop.
> -  int IsOutermostParallel;
> +class IslAstInfo : public ScopPass {
> +public:
> +  /// @brief Payload information used to annoate an ast node

Finish with a '.'.

> +  struct IslAstUserPayload {
> +    /// @brief Construct and initialize the payload.
> +    IslAstUserPayload()
> +        : IsInnermostParallel(false), IsOutermostParallel(false),
> +          IsReductionParallel(false), Build(nullptr) {}
>
> -  // The node is the innermost parallel loop.
> -  int IsInnermostParallel;
> +    /// @brief Cleanup all isl structs on destruction.
> +    ~IslAstUserPayload();
>
> -  // The node is only parallel because of reductions
> -  bool IsReductionParallel;
> -};
> +    /// @brief Flag to mark innermost parallel loops.
> +    bool IsInnermostParallel;
>
> -class IslAstInfo : public ScopPass {
> +    /// @brief Flag to mark outermost parallel loops.
> +    bool IsOutermostParallel;
> +
> +    /// @brief Flag to mark reduction parallel loops.
> +    bool IsReductionParallel;
> +
> +    /// @brief The build environment at the time this node was
constructed
> +    isl_ast_build *Build;
> +  };
> +
> +private:
>     Scop *S;
>     IslAst *Ast;
>
> @@ -97,6 +109,12 @@ public:
>     /// @brief Is this loop a reduction parallel loop?
>     static bool isReductionParallel(__isl_keep isl_ast_node *Node);
>
> +  /// @brief Get the nodes context or a nullptr if not available
Finish with a "."

> +  static __isl_give isl_ast_build *getBuild(__isl_keep isl_ast_node 
> + *Node);
> +
> +  /// @brief Get the nodes schedule or a nullptr if not available
Finish with a "."

> +  static __isl_give isl_union_map *getSchedule(__isl_keep 
> + isl_ast_node *Node);
> +
>     ///}
>
>     virtual void getAnalysisUsage(AnalysisUsage &AU) const; diff --git 
> a/lib/CodeGen/IslAst.cpp b/lib/CodeGen/IslAst.cpp index
> ac41c91..4cc11e4 100644
> --- a/lib/CodeGen/IslAst.cpp
> +++ b/lib/CodeGen/IslAst.cpp
> @@ -34,10 +34,12 @@
>   #include "isl/map.h"
>   #include "isl/aff.h"
>
> +#define DEBUG_TYPE "polly-ast"
> +
>   using namespace llvm;
>   using namespace polly;
>
> -#define DEBUG_TYPE "polly-ast"
> +using IslAstUserPayload = IslAstInfo::IslAstUserPayload;
>
>   static cl::opt<bool> UseContext("polly-ast-use-context",
>                                   cl::desc("Use context"), cl::Hidden, 
> @@ -69,10 +71,19 @@ private:
>     isl_ast_node *Root;
>     isl_ast_expr *RunCondition;
>
> -  void buildRunCondition(__isl_keep isl_ast_build *Context);
> +  void buildRunCondition(__isl_keep isl_ast_build *Build);
>   };
>   } // End namespace polly.
>
> +/// @brief Free an IslAstUserPayload object pointed to by @p Ptr 
> +static void freeIslAstUserPayload(void *Ptr) {
> +  delete ((IslAstInfo::IslAstUserPayload *)Ptr); }
> +
> +IslAstInfo::IslAstUserPayload::~IslAstUserPayload() {
> +  isl_ast_build_free(Build);
> +}
> +
>   // Temporary information used when building the ast.
>   struct AstBuildUserInfo {
>     // The dependence information.
> @@ -115,32 +126,12 @@ printFor(__isl_take isl_printer *Printer,
>     if (!Id)
>       return isl_ast_node_for_print(Node, Printer, PrintOptions);
>
> -  struct IslAstUserPayload *Info =
> -      (struct IslAstUserPayload *)isl_id_get_user(Id);
> +  IslAstUserPayload *Info = (IslAstUserPayload *)isl_id_get_user(Id);
>     Printer = printParallelFor(Node, Printer, PrintOptions, Info);
>     isl_id_free(Id);
>     return Printer;
>   }
>
> -// Allocate an AstNodeInfo structure and initialize it with default
values.
> -static struct IslAstUserPayload *allocateIslAstUser() {
> -  struct IslAstUserPayload *NodeInfo;
> -  NodeInfo =
> -      (struct IslAstUserPayload *)malloc(sizeof(struct
IslAstUserPayload));
> -  NodeInfo->Context = 0;
> -  NodeInfo->IsOutermostParallel = 0;
> -  NodeInfo->IsInnermostParallel = 0;
> -  NodeInfo->IsReductionParallel = false;
> -  return NodeInfo;
> -}
> -
> -// Free the AstNodeInfo structure.
> -static void freeIslAstUser(void *Ptr) {
> -  struct IslAstUserPayload *UserStruct = (struct IslAstUserPayload 
> *)Ptr;
> -  isl_ast_build_free(UserStruct->Context);
> -  free(UserStruct);
> -}
> -
>   // Check if the current scheduling dimension is parallel.
>   //
>   // We check for parallelism by verifying that the loop does not 
> carry any @@ -221,8 +212,8 @@ static bool 
> astScheduleDimIsParallel(__isl_keep isl_ast_build *Build,
>
>   // Mark a for node openmp parallel, if it is the outermost parallel for
node.
>   static void markOpenmpParallel(__isl_keep isl_ast_build *Build,
> -                               struct AstBuildUserInfo *BuildInfo,
> -                               struct IslAstUserPayload *NodeInfo) {
> +                               AstBuildUserInfo *BuildInfo,
> +                               IslAstUserPayload *NodeInfo) {
>     if (BuildInfo->InParallelFor)
>       return;
>
> @@ -242,10 +233,10 @@ static void markOpenmpParallel(__isl_keep
isl_ast_build *Build,
>   //
>   static __isl_give isl_id *astBuildBeforeFor(__isl_keep isl_ast_build
*Build,
>                                               void *User) {
> -  struct AstBuildUserInfo *BuildInfo = (struct AstBuildUserInfo 
> *)User;
> -  struct IslAstUserPayload *NodeInfo = allocateIslAstUser();
> +  AstBuildUserInfo *BuildInfo = (AstBuildUserInfo *)User; 
> + IslAstUserPayload *NodeInfo = new IslAstUserPayload();
>     isl_id *Id = isl_id_alloc(isl_ast_build_get_ctx(Build), "", 
> NodeInfo);
> -  Id = isl_id_set_free_user(Id, freeIslAstUser);
> +  Id = isl_id_set_free_user(Id, freeIslAstUserPayload);
>
>     markOpenmpParallel(Build, BuildInfo, NodeInfo);
>
> @@ -305,9 +296,8 @@ astBuildAfterFor(__isl_take isl_ast_node *Node,
__isl_keep isl_ast_build *Build,
>     isl_id *Id = isl_ast_node_get_annotation(Node);
>     if (!Id)
>       return Node;
> -  struct IslAstUserPayload *Info =
> -      (struct IslAstUserPayload *)isl_id_get_user(Id);
> -  struct AstBuildUserInfo *BuildInfo = (struct AstBuildUserInfo 
> *)User;
> +  IslAstUserPayload *Info = (IslAstUserPayload *)isl_id_get_user(Id); 
> + AstBuildUserInfo *BuildInfo = (AstBuildUserInfo *)User;
>
>     if (Info) {
>       if (Info->IsOutermostParallel)
> @@ -316,8 +306,8 @@ astBuildAfterFor(__isl_take isl_ast_node *Node,
__isl_keep isl_ast_build *Build,
>         if (astScheduleDimIsParallel(Build, BuildInfo->Deps,
>                                      Info->IsReductionParallel))
>           Info->IsInnermostParallel = 1;
> -    if (!Info->Context)
> -      Info->Context = isl_ast_build_copy(Build);
> +    if (!Info->Build)
> +      Info->Build = isl_ast_build_copy(Build);
>     }
>
>     isl_id_free(Id);
> @@ -325,29 +315,29 @@ astBuildAfterFor(__isl_take isl_ast_node *Node,
__isl_keep isl_ast_build *Build,
>   }
>
>   static __isl_give isl_ast_node *AtEachDomain(__isl_take isl_ast_node
*Node,
> -                                             __isl_keep isl_ast_build
*Context,
> +                                             __isl_keep isl_ast_build 
> + *Build,
>                                                void *User) {
> -  struct IslAstUserPayload *Info = nullptr;
> +  IslAstUserPayload *Info = nullptr;
>     isl_id *Id = isl_ast_node_get_annotation(Node);
>
>     if (Id)
> -    Info = (struct IslAstUserPayload *)isl_id_get_user(Id);
> +    Info = (IslAstUserPayload *)isl_id_get_user(Id);
>
>     if (!Info) {
>       // Allocate annotations once: parallel for detection might have
already
>       // allocated the annotations for this node.
> -    Info = allocateIslAstUser();
> +    Info = new IslAstUserPayload();
>       Id = isl_id_alloc(isl_ast_node_get_ctx(Node), nullptr, Info);
> -    Id = isl_id_set_free_user(Id, &freeIslAstUser);
> +    Id = isl_id_set_free_user(Id, freeIslAstUserPayload);
>     }
>
> -  if (!Info->Context)
> -    Info->Context = isl_ast_build_copy(Context);
> +  if (!Info->Build)
> +    Info->Build = isl_ast_build_copy(Build);
>
>     return isl_ast_node_set_annotation(Node, Id);
>   }

>   IslAst::~IslAst() {
> @@ -476,6 +466,16 @@ bool IslAstInfo::isReductionParallel(__isl_keep
isl_ast_node *Node) {
>     return Payload && Payload->IsReductionParallel;
>   }
>
> +isl_ast_build *IslAstInfo::getBuild(__isl_keep isl_ast_node *Node) {
> +  IslAstUserPayload *Payload = getNodePayload(Node);
> +  return Payload ? isl_ast_build_copy(Payload->Build) : nullptr; }

This function is unused. Maybe we should not add it at all.

> +isl_union_map *IslAstInfo::getSchedule(__isl_keep isl_ast_node *Node) 
> +{
__isl_give is missing

> +  IslAstUserPayload *Payload = getNodePayload(Node);
> +  return Payload ? isl_ast_build_get_schedule(Payload->Build) : 
> +nullptr; }
> +
>   void IslAstInfo::printScop(raw_ostream &OS) const {
>     isl_ast_print_options *Options;
>     isl_ast_node *RootNode = getAst(); diff --git 
> a/lib/CodeGen/IslCodeGeneration.cpp
> b/lib/CodeGen/IslCodeGeneration.cpp
> index 4eb39cc..142324c 100644
> --- a/lib/CodeGen/IslCodeGeneration.cpp
> +++ b/lib/CodeGen/IslCodeGeneration.cpp
> @@ -778,20 +778,8 @@ IslNodeBuilder::getUpperBound(__isl_keep isl_ast_node
*For,
>   }
>
>   unsigned IslNodeBuilder::getNumberOfIterations(__isl_keep
> isl_ast_node *For) {
> -  isl_id *Annotation = isl_ast_node_get_annotation(For);
> -  if (!Annotation)
> -    return -1;
> -
> -  struct IslAstUserPayload *Info =
> -      (struct IslAstUserPayload *)isl_id_get_user(Annotation);
> -  if (!Info) {
> -    isl_id_free(Annotation);
> -    return -1;
> -  }
> -
> -  isl_union_map *Schedule =
> isl_ast_build_get_schedule(Info->Context);
> +  isl_union_map *Schedule = IslAstInfo::getSchedule(Build);
>     isl_set *LoopDomain =
> isl_set_from_union_set(isl_union_map_range(Schedule));
> -  isl_id_free(Annotation);
>     int NumberOfIterations = polly::getNumberOfIterations(LoopDomain);
>     if (NumberOfIterations == -1)
>       return -1;
> @@ -848,14 +836,7 @@ void IslNodeBuilder::createForVector(__isl_take
isl_ast_node *For,
>     for (int i = 1; i < VectorWidth; i++)
>       IVS[i] = Builder.CreateAdd(IVS[i - 1], ValueInc, "p_vector_iv");
>
> -  isl_id *Annotation = isl_ast_node_get_annotation(For);
> -  assert(Annotation && "For statement is not annotated");
> -
> -  struct IslAstUserPayload *Info =
> -      (struct IslAstUserPayload *)isl_id_get_user(Annotation);
> -  assert(Info && "For statement annotation does not contain info");
> -
> -  isl_union_map *Schedule =
> isl_ast_build_get_schedule(Info->Context);
> +  isl_union_map *Schedule = IslAstInfo::getSchedule(Build);

Nice cleanup here.