This is an archive of the discontinued LLVM Phabricator instance.

[Polly][Refactor] Generalize parallel code generation
ClosedPublic

Authored by jdoerfert on Aug 20 2014, 11:11 AM.

Details

Summary
+ Generalized function names and comments
  + Removed OpenMP (omp) from the names and comments
  + Use common names (non OpenMP specific) for runtime library call creation
    methodes
+ Commented the parallel code generator and all its member functions
+ Refactored some values and methodes

Diff Detail

Repository
rL LLVM

Event Timeline

jdoerfert updated this revision to Diff 12708.Aug 20 2014, 11:11 AM
jdoerfert retitled this revision from to [Polly][Refactor] Generalize parallel code generation.
jdoerfert updated this object.
jdoerfert added reviewers: grosser, sebpop, simbuerg.
jdoerfert added subscribers: Restricted Project, Unknown Object (MLST).
grosser edited edge metadata.Oct 1 2014, 8:57 AM

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.

include/polly/CodeGen/LoopGenerators.h
63 ↗(On Diff #12708)

Nice comments!

65 ↗(On Diff #12708)

Polly-Threads does not exist in the public Polly version.

94 ↗(On Diff #12708)

Nice documentation!

114 ↗(On Diff #12708)

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.

159 ↗(On Diff #12708)

Missing points at the end of the sentences.

lib/CodeGen/CodeGeneration.cpp
597 ↗(On Diff #12708)

C++11 range based loop?

lib/CodeGen/LoopGenerators.cpp
226 ↗(On Diff #12708)

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.

296 ↗(On Diff #12708)

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)

299 ↗(On Diff #12708)

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.

369 ↗(On Diff #12708)

We always seem to use this code path in polly, so adding this condition does not seem necessary.

test/Cloog/CodeGen/OpenMP/simple_nested_loop.ll
82 ↗(On Diff #12708)

Can we just use "polly.par.userContext" instead of this regex stuff. That seems a lot more readable to me.

90 ↗(On Diff #12708)

Can we just use the full name "loop_openmp.polly.subfn"?

jdoerfert updated this revision to Diff 14287.Oct 1 2014, 9:43 AM
jdoerfert edited edge metadata.

Minor change

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.

jdoerfert updated this revision to Diff 14330.Oct 2 2014, 10:05 AM

Removed semantic changes and updated according to comments

I found one last issue where the original code was not very clear and your new changes unfortunately make this even more unclear (or even incorrect).

Otherwise, LGTM.

include/polly/CodeGen/LoopGenerators.h
102 ↗(On Diff #14330)

Point at the end is missing.

137 ↗(On Diff #14330)

dominance, you miss an 'n'

140 ↗(On Diff #14330)

This type is not used to create the IVs, but rather to declare/call the OpenMP functions we use. For an OpenMP function declared as "bool GOMP_loop_runtime_next(long, long)", we want to make sure that the integer type we use to declare it at LLVM-IR is identical in size to the type 'long' is lowered to on a certain platform. On linux and OS X the size of this type is equivalent to the pointer type.

See this discussion:

http://lists.cs.uiuc.edu/pipermail/llvmdev/2010-December/037118.html

Your rename from from LongTy to IVType makes this less obvious.

Also asking the user (lib/CodeGeneration.cpp) to pass in the LongTy currently works, as the CLooG code generation uses the same type for IVs, we do not do this in the isl code generation. In the isl code generation we currently hardcode 64 bit IVs and later we will hopefully derive the optimal time automatically (hence it may vary).

jdoerfert updated this revision to Diff 14385.Oct 3 2014, 9:48 AM

Fixed the long pointer type issue and two typos

jdoerfert updated this revision to Diff 14386.Oct 3 2014, 10:02 AM

Fixed long type issue finally

grosser accepted this revision.Oct 3 2014, 10:35 AM
grosser edited edge metadata.

I like the idea with the DataLayout. This makes the behaviour a lot more clear. Thanks for insisting here.

You also sneaked in a semantic change without a test case (actually it can not be tested as the current CLooG code generator always generates matching types making this basically dead code). I would feel more comfortable if we remove such asserts only when needed and with a specific test case and explaining why truncating is OK, instead of having this part of a larger refactoring.

​ if (LB->getType() != LongType)
​ LB = Builder.CreateSExtOrTrunc(LB, LongType);
​ if (UB->getType() != LongType)
​ UB = Builder.CreateSExtOrTrunc(UB, LongType);
​ if (Stride->getType() != LongType)
​ Stride = Builder.CreateSExtOrTrunc(Stride, LongType);

Otherwise LGTM.

This revision is now accepted and ready to land.Oct 3 2014, 10:35 AM
In D4990#17, @grosser wrote:

I like the idea with the DataLayout. This makes the behaviour a lot more clear. Thanks for insisting here.

You also sneaked in a semantic change without a test case (actually it can not be tested as the current CLooG code generator always generates matching types making this basically dead code). I would feel more comfortable if we remove such asserts only when needed and with a specific test case and explaining why truncating is OK, instead of having this part of a larger refactoring.

​ if (LB->getType() != LongType)
​ LB = Builder.CreateSExtOrTrunc(LB, LongType);
​ if (UB->getType() != LongType)
​ UB = Builder.CreateSExtOrTrunc(UB, LongType);
​ if (Stride->getType() != LongType)
​ Stride = Builder.CreateSExtOrTrunc(Stride, LongType);

What exactly is your problem here?

The type asserts were introduced by my patch and are replaced by this
because we need this anyway. What else do we want to do if type(LB) <
LongType or type(LB) > LongType?

jdoerfert closed this revision.Oct 3 2014, 12:20 PM
jdoerfert updated this revision to Diff 14393.

Closed by commit rL219003 (authored by @jdoerfert).