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

[Johannes Doerfert]

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?

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

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

(also documentation)

================
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?

================
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?

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

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

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

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

If you agree, then I suggest to use the union_set_foreach_set only to collect all Stmts/BBs inside of For and then iterate again over them. This time we do the instruction operand checks but we check if the parent of an instruction operand is in the set of basic blocks we collected earlier. This way we should be able to identify all inner SCoP instructions we need to pass as arguments.

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

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

================
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.
Or even better we merge the two loops and use this lookup to decide what action to take. But now it looks to good to be true, what did I miss?

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

================
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?





================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:446
@@ +445,3 @@
+}
+
+void IslNodeBuilder::createForOpenMP(__isl_take isl_ast_node *For) {
----------------
I stoped reviewing here, will continue tonight.

http://reviews.llvm.org/D5517