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

[Johannes Doerfert]

Some "final" comments. Maybe you want to address some of them. If LNT looks good I would be fine with this patch.

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:110
@@ -97,1 +109,3 @@
 
+  // A set of Value -> Value remappings to apply when generating new code.
+  //
----------------
one more / please

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:150
@@ +149,3 @@
+  /// Loops that contain the scop or that are part of the scop are considered
+  /// locally defined.
+  ///
----------------
What loop are left?

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:176
@@ -124,2 +175,3 @@
   void createForSequential(__isl_take isl_ast_node *For);
+  void createForParallel(__isl_take isl_ast_node *For);
 
----------------
docu?

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:308
@@ +307,3 @@
+  isl_id *Id = isl_set_get_tuple_id(Set);
+  struct FindValuesUser *U = static_cast<struct FindValuesUser *>(User);
+  const ScopStmt *Stmt = static_cast<const ScopStmt *>(isl_id_get_user(Id));
----------------
The name is not really descriptive.

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:356
@@ +355,3 @@
+
+  Values.remove_if([this](const Value *V) { return isa<GlobalValue>(V); });
+
----------------
I'm still not really convinced this is better/nicer than a static function.

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:360
@@ +359,3 @@
+    return this->S.getRegion().contains(L) ||
+           L->contains(S.getRegion().getEntry());
+  });
----------------
I don't understand this. Can you add a comment, otherwise this is a magic condition that cannot be maintained or debuged.

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:366
@@ +365,3 @@
+    ParallelLoopGenerator::ValueToValueMapTy &NewValues) {
+  std::set<Value *> Inserted;
+
----------------
I usually like the SmallPtrSet but that's taste I guess.

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:555
@@ +554,3 @@
+/// @param DT The dominator tree from which to remove the BBs.
+static void removeSubFuncFromDomTree(Function *F, DominatorTree &DT) {
+  DomTreeNode *N = DT.getNode(&F->getEntryBlock());
----------------
What about a flag to the loopgenerator such that it will not add these blocks to the DT at all (or just omit the DT argument)?

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:845
@@ +844,3 @@
+  // scop itself, but as the number of such scops may be arbitrarily large we do
+  // not generate code for references to them ahead of time.
+  Region &R = S.getRegion();
----------------
I don't understand the comment? e.g., What do you mean by ahead of time? When do we generate the references then?

================
Comment at: lib/Support/SCEVValidator.cpp:365
@@ -364,1 +364,3 @@
 
+struct SCEVFindLoops : public SCEVVisitor<SCEVFindLoops, void> {
+  SetVector<const Loop *> &Loops;
----------------
There is another type of "SCEVVisitor", Sebastian pointed it out to me once. IT only has one function something like:
  visit(const SCEV *S);
and you cast the SCEV yourself. I would suggest using that for visitors that are only interested in one or two SCEV types.

================
Comment at: lib/Support/SCEVValidator.cpp:431
@@ +430,3 @@
+
+struct SCEVFindUnknowns : public SCEVVisitor<SCEVFindUnknowns, void> {
+  SetVector<Value *> &Unknowns;
----------------
Same as above

http://reviews.llvm.org/D5517