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

Wed Oct 1 09:50:16 PDT 2014

I will restructure the patch and split it. Give me a day or two.
However, we should probably apply it before the OpenMP patch for the
IslCodeGeneration and use similar naming conventions there.

On 10/01, Tobias Grosser wrote:
> 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
> 
> 

-- 

Johannes Doerfert
Researcher / PhD Student

Compiler Design Lab (Prof. Hack)
Saarland University, Computer Science
Building E1.3, Room 4.26

Tel. +49 (0)681 302-57521 : doerfert at cs.uni-saarland.de
Fax. +49 (0)681 302-3065  : http://www.cdl.uni-saarland.de/people/doerfert
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141001/78c6e95c/attachment.sig>