[PATCH] [Polly][Refactor] Generalize parallel code generation

[Tobias Grosser]

Hi Johannes,

thanks for this patch. It adds some nice documentation and as far as I understand also helps out of tree users who do not use OpenMP. The only technical concern I have is that you combine a larger documentation/refactoring change with some actual semantical changes. I would prefer to not have semantical changes in such patches. Not even mentioning them in the commit message makes them even more surprising.

================
Comment at: include/polly/CodeGen/LoopGenerators.h:63
@@ +62,3 @@
+///      to the struct holding all needed values.
+///
+/// At the moment we support two runtimes, OpenMP and Polly-Threads (a pthread
----------------
Nice comments!

================
Comment at: include/polly/CodeGen/LoopGenerators.h:65
@@ +64,3 @@
+/// At the moment we support two runtimes, OpenMP and Polly-Threads (a pthread
+/// wrapper).
+///
----------------
Polly-Threads does not exist in the public Polly version.

================
Comment at: include/polly/CodeGen/LoopGenerators.h:94
@@ +93,3 @@
+///     cleanup_thread();
+///   }
+class ParallelLoopGenerator {
----------------
Nice documentation!

================
Comment at: include/polly/CodeGen/LoopGenerators.h:114
@@ -54,1 +113,3 @@
+  Module *M;
+
 public:
----------------
I normally try to have public functions first to make them visible to the user of a class. Private implementation details are kept later.

I don't feel super strong about this.

================
Comment at: include/polly/CodeGen/LoopGenerators.h:159
@@ +158,3 @@
+  /// @param UB         The upper bound for the loop we parallelize
+  /// @param Stride     The stride of the loop we parallelize
+  void createCallSpawnThreads(Value *SubFn, Value *SubFnParam, Value *LB,
----------------
Missing points at the end of the sentences.

================
Comment at: lib/CodeGen/CodeGeneration.cpp:597
@@ -598,1 +596,3 @@
+  for (ParallelLoopGenerator::ValueToValueMapTy::iterator I = VMap.begin(),
+                                                          E = VMap.end();
        I != E; ++I) {
----------------
C++11 range based loop?

================
Comment at: lib/CodeGen/LoopGenerators.cpp:226
@@ +225,3 @@
+void ParallelLoopGenerator::createCallJoinThreads() {
+  const std::string Name = "GOMP_parallel_end";
+
----------------
Is there a benefit of switching to std::string? I looked for why you would use it for string literals, but could only find this discussion which is not in favor: http://stackoverflow.com/questions/2099731/which-one-to-use-const-char-or-const-stdstring

I don't feel strongly about it, but this seems to be a change not really related to 'generalizing' the OMPGenerator. As such, I would generally leave it out to keep the diff small and simplify review. Also, if you see a benefit in this change, I would appreciate a explanation of why it is better such that others can follow your reasoning in the future.

================
Comment at: lib/CodeGen/LoopGenerators.cpp:296
@@ -245,2 +295,3 @@
   StructType *Ty = StructType::get(Builder.getContext(), Members);
-  Value *Struct = Builder.CreateAlloca(Ty, 0, "omp.userContext");
+  Value *Struct = new AllocaInst(Ty, 0, "polly.par.userContext", IP);
+
----------------
This is a useful change, but hiding it inside such a huge refactoring change is not a good idea.

To make this refactoring easy to reason about, it should not contain any non-trivial functional changes.

We probably also need a test case for this (in a separate patch)

================
Comment at: lib/CodeGen/LoopGenerators.cpp:299
@@ -247,1 +298,3 @@
+  ConstantInt *SizeOf = dyn_cast<ConstantInt>(ConstantExpr::getSizeOf(Ty));
+  Builder.CreateLifetimeStart(Struct, SizeOf);
 
----------------
The same holds for this change. Creating lifetime markers is a good idea, but I would prefer to do this in a separate patch that includes a targeted test case.

================
Comment at: lib/CodeGen/LoopGenerators.cpp:369
@@ -320,1 +368,3 @@
+    UB = Builder.CreateSub(UB, ConstantInt::get(IntPtrTy, 1),
+                           "polly.par.UBAdjusted");
 
----------------
We always seem to use this code path in polly, so adding this condition does not seem necessary.

================
Comment at: test/Cloog/CodeGen/OpenMP/simple_nested_loop.ll:82
@@ +81,3 @@
+; CHECK: %[[PN:[._a-zA-Z0-9]*]]userContext = alloca { i32 }
+; CHECK: %[[NO:[._a-zA-Z0-9]*]] = getelementptr inbounds { i32 }* %[[PN]]userContext, i32 0, i32 0
+; CHECK: store i32 %polly.indvar, i32* %[[NO]]
----------------
Can we just use "polly.par.userContext" instead of this regex stuff. That seems a lot more readable to me.

================
Comment at: test/Cloog/CodeGen/OpenMP/simple_nested_loop.ll:90
@@ -89,3 +89,3 @@
 ; Verify the new subfunction is annotated such that SCoP detection will skip it.
-; CHECK: @loop_openmp.omp_subfn({{.*}}) [[ATTR:#[0-9]+]]
+; CHECK: @loop_openmp{{.*}}subfn({{.*}}) [[ATTR:#[0-9]+]]
 ; CHECK: attributes [[ATTR]] = {{{[^\}]*}}polly.skip.fn{{[^\}]*}}}
----------------
Can we just use the full name "loop_openmp.polly.subfn"?

http://reviews.llvm.org/D4990