[PATCH] [Polly] [IslCodeGenerator] Add OpenMP support

[Tobias Grosser]

On 29.09.2014 15:12, Johannes Doerfert wrote:
> Reviewed half of it but have to go now
>
> ================
> Comment at: include/polly/CodeGen/IslExprBuilder.h:80
> @@ -79,3 +79,3 @@
>     /// @brief A map from isl_ids to llvm::Values.
> -  typedef std::map<isl_id *, llvm::Value *> IDToValueTy;
> +  typedef llvm::MapVector<isl_id *, llvm::Value *> IDToValueTy;
>
> ----------------
> This change can/should be commited earlier, LGTM btw. but is there a particular reason for MapVector instead of SmallDenseMap or just DenseMap?

This change is only needed starting from this commit. Before we do not 
iterate over the content of this map. Hence, I would like to leave it in 
this patchset.

(I extracted out a couple of other independent change sets)

> ================
> Comment at: lib/CodeGen/IslCodeGeneration.cpp:89
> @@ +88,3 @@
> +  // llvm::Values.
> +  ValueMapT ValueMap;
> +
> ----------------
> I don't like the comment (style + content). It doesn't give any more information than the type of the map.

I extended the comment.

> ================
> Comment at: lib/CodeGen/IslCodeGeneration.cpp:114
> @@ -106,1 +113,3 @@
>     unsigned getNumberOfIterations(__isl_keep isl_ast_node *For);
> +  SetVector<Value *> getOMPValues(__isl_keep isl_ast_node *For);
> +  void updateWithValueMap(OMPGenerator::ValueToValueMapTy &VMap);
> ----------------
> Please, do not use this awfull OMP any more but talk about parallel function arguments or sth. similar.

Removed reference to OMP.

> ================
> Comment at: lib/CodeGen/IslCodeGeneration.cpp:115
> @@ -107,1 +114,3 @@
> +  SetVector<Value *> getOMPValues(__isl_keep isl_ast_node *For);
> +  void updateWithValueMap(OMPGenerator::ValueToValueMapTy &VMap);
>
> ----------------
> When I see the type here I remember my patch to generalize the code generation (LoopGenerators). I hoped you would review that and work on that as a base. Any reasons not to?

The new patch set is based on your generalized code generator (which 
itself was already committed).

> ================
> Comment at: lib/CodeGen/IslCodeGeneration.cpp:120
> @@ -110,2 +119,3 @@
>     void createForSequential(__isl_take isl_ast_node *For);
> +  void createForOpenMP(__isl_take isl_ast_node *For);
>
> ----------------
> isn't createForParallel much nicer?

Changed.

> ================
> Comment at: lib/CodeGen/IslCodeGeneration.cpp:237
> @@ -226,1 +236,3 @@
>
> +SetVector<Value *> IslNodeBuilder::getOMPValues(__isl_keep isl_ast_node *For) {
> +  SetVector<Value *> Values;
> ----------------
> I usually do not like this Object producing methodes, but prefere the object to be passed as reference instead, especially for Vectors/Sets/Maps/etc.

Changed.

> ================
> Comment at: lib/CodeGen/IslCodeGeneration.cpp:245
> @@ +244,3 @@
> +
> +  auto Func = [](isl_set *Set, void *User) {
> +    isl_id *Id = isl_set_get_tuple_id(Set);
> ----------------
> Any reason not to make this a static function? I'm not sure about such lambda's myself and I think we always used static functions so far.

Changed.

> ================
> Comment at: lib/CodeGen/IslCodeGeneration.cpp:247
> @@ +246,3 @@
> +    isl_id *Id = isl_set_get_tuple_id(Set);
> +    SetVector<Value *> &Values = *static_cast<SetVector<Value *> *>(User);
> +    const ScopStmt *Stmt = static_cast<const ScopStmt *>(isl_id_get_user(Id));
> ----------------
> I don't really see the benefit of static_cast here as User is a black box void*, however it might be good to start using static_cast instead of C-style casts all the time.

I use C++ style casts to make clarify the semantics of this cast.

> ================
> Comment at: lib/CodeGen/IslCodeGeneration.cpp:255
> @@ +254,3 @@
> +        if (Instruction *OpInst = dyn_cast<Instruction>(SrcVal))
> +          if (Stmt->getParent()->getRegion().contains(OpInst))
> +            continue;
> ----------------
> That is probably not going to work. The instruction might be in the SCoP but outside the parallel loop, thus still needed as an argument for the parallel subfunction.

Instructions inside the ScoP, but outside the parallel loop should have 
been already promote to through-memory dependences. Only values defined
outside the scop have not been converted, as they can remain unchanged
as long as all code remains within the same function.

> ================
> Comment at: lib/CodeGen/IslCodeGeneration.cpp:258
> @@ +257,3 @@
> +
> +        if (isa<Instruction>(SrcVal) || isa<Argument>(SrcVal))
> +          Values.insert(SrcVal);
> ----------------
> I would like the negated check better:
>    if (isa<Constant> || isa<Global>)
> or
>    canSynthesise(...)
> then continue, otherwise assume we need it.

I use canSynthesise now.

> ================
> Comment at: lib/CodeGen/IslCodeGeneration.cpp:273
> @@ +272,3 @@
> +void IslNodeBuilder::updateWithValueMap(OMPGenerator::ValueToValueMapTy &VMap) {
> +  std::set<Value *> Inserted;
> +
> ----------------
> Why back to std::set?

I don't understand your comment.

> ================
> Comment at: lib/CodeGen/IslCodeGeneration.cpp:281
> @@ +280,3 @@
> +  for (const auto &I : VMap) {
> +    if (Inserted.count(I.first))
> +      continue;
> ----------------
> We could remove the need for the extra set by looking up I.first in IDToValue, if found we continue otherwise we go on.

This does not work. I.first is a llvm::Value, but IDToValue has an 
isl_id as key. When reading your comment I also got first confused due 
to the I.first in both loops having different types.

> ================
> Comment at: lib/CodeGen/IslCodeGeneration.cpp:439
> @@ +438,3 @@
> +  DomTreeNode *N = DT.getNode(&F->getEntryBlock());
> +  std::vector<BasicBlock *> Nodes;
> +  for (po_iterator<DomTreeNode *> I = po_begin(N), E = po_end(N); I != E; ++I)
> ----------------
> Back to stdlib...

I do not understand this comment.

> ================
> Comment at: lib/CodeGen/IslCodeGeneration.cpp:440
> @@ +439,3 @@
> +  std::vector<BasicBlock *> Nodes;
> +  for (po_iterator<DomTreeNode *> I = po_begin(N), E = po_end(N); I != E; ++I)
> +    Nodes.push_back(I->getBlock());
> ----------------
> This is hard to understand. If I got it correctly (but please tell me if I'm wrong), you collect all nodes of a function in post order in a set and erase them afterwards from the dominator tree of the function, right?

I added a comment explaining this.

Thanks again for your detailed review.

Cheers,
Tobias