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

[Tobias Grosser]

In the previous patch, I addressed most reviewer comments.

Two points where I did not follow reviewer suggestions:

- I kept the small lambda function.
- I clean up the dominance tree at the end to stay in sync with CLooG (but added a FIXME to possibly improve on this later)

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:90
@@ -83,1 +89,3 @@
 
+  /// @brief The current iteration of out of scop loops
+  ///
----------------
simbuerg wrote:
> out-of-scop
Fixed.

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

================
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.
+  ///
----------------
jdoerfert wrote:
> What loop are left?
I added:

"Loops that are before the scop, but do not contain the  scop itself are considered not locally defined."


================
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);
 
----------------
jdoerfert wrote:
> docu?
I added:

+  /// Create LLVM-IR that executes a for node thread parallel.
+  ///
+  /// @param For The FOR isl_ast_node for which code is generated.


================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:301
@@ +300,3 @@
+
+/// Extract the values and SCEVs needed to code generate a ScopStmt.
+///
----------------
simbuerg wrote:
> Extract the values and SCEVs needed to generate code for a ScopStmt.
Fixed.

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:305
@@ +304,3 @@
+/// Values this statement depends on as well as a set of SCEV expressions that
+/// need to be synthesized when code generating this statment.
+static int findValuesInStmt(isl_set *Set, void *User) {
----------------
simbuerg wrote:
> ... when generating code for this statement.
Fixed.

================
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));
----------------
jdoerfert wrote:
> The name is not really descriptive.
I forgot to add this to the patch, but I now renamed this to

void *UserPtr and struct FindValuesUser *User

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:356
@@ +355,3 @@
+
+  Values.remove_if([this](const Value *V) { return isa<GlobalValue>(V); });
+
----------------
simbuerg wrote:
> jdoerfert wrote:
> > I'm still not really convinced this is better/nicer than a static function.
> Most likely the static function would get a descriptive name, such as: isGlobalValue, right? That is quite redundant given the only statement this function executes is: return isa<GlobalValue>(V);
> 
> Furthermore, I do not think that this static function would get called from somewhere else in here, so whats the point in having it around on the top-level.
I left the lambda for now.

Similar to Andreas I have the impression that this as good as we can find a use case for lambdas. Are you proposing to generally avoid lambdas?

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:360
@@ +359,3 @@
+    return this->S.getRegion().contains(L) ||
+           L->contains(S.getRegion().getEntry());
+  });
----------------
jdoerfert wrote:
> I don't understand this. Can you add a comment, otherwise this is a magic condition that cannot be maintained or debuged.
This was already indirectly documented in the header of the surrounding function. For clarity I added a brief comment right above the stmt:

+  /// Remove loops that contain the scop or that are part of the scop, as they
+  /// are considered local. This leaves only loops that are before the scop, but
+  /// do not contain the scop itself.


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

================
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());
----------------
jdoerfert wrote:
> 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)?
Interesting idea. This requires some work to understand if this is possible. I added a FIXME. For now I propose to use the same solution as we had in the CLooG backend.

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:615
@@ +614,3 @@
+  // Create for all loops we depend on values that contain the current loop
+  // iteration. These values are necessary to code generate SCEVs that depend on
+  // such loops. As a result we need to pass them to the subfunction.
----------------
simbuerg wrote:
> ... generate code for SCEVs...
Fixed.

================
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();
----------------
jdoerfert wrote:
> I don't understand the comment? e.g., What do you mean by ahead of time? When do we generate the references then?
Changed to:

+  // We may also reference loops outside of the scop which do not contain the
+  // scop itself, but as the number of such scops may be arbitrarily large we do
+  // not generate code for them here, but only at the point of code generation
+  // where these values are needed.


================
Comment at: lib/Support/SCEVValidator.cpp:365
@@ -364,1 +364,3 @@
 
+struct SCEVFindLoops : public SCEVVisitor<SCEVFindLoops, void> {
+  SetVector<const Loop *> &Loops;
----------------
simbuerg wrote:
> jdoerfert wrote:
> > 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.
> Nice find, that simplifies so many things
Changed. This really reduced the code needed here significantly.

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

http://reviews.llvm.org/D5517