[PATCH] [Polly][Refactor] Cleanup runtime code generation

[Tobias Grosser]

Thanks Johannes for the update.

I like the solution with adapting the simplifyRegion function. It avoids the regression the earlier patch introduced and still allows us to build the run-time condition before splitting the region.

Some minor points that are still open (some mentioned before):

- You talk twice about "runtime code generation". This term seems wrong. I suppose you mean "run-time-check
  generation"?

- It would be great if the commit message contains a brief description of the changes you applied, possibly
  mentioning the need to ensure an unconditional entry edge.

- You still have the regexp change in this commit. I would prefer if you do not apply them in this commit, as they hide the actual code changes.  You can commit them immediately after, if you like. (In fact, if you add some checks on the bb labels as well, the tests will really nicely document your changes)
  
Two more areas where I would like to learn more about your motivations (both not blocking the commit), to understand if/what I should pay attention to when writing patches myself:

- Why is moving variables to class scope better. Because we can document them there?

- I would like to understand in which situations we needed REGEXPs? For all checks? Only unnamed checks?

================
Comment at: include/polly/CodeGen/Utils.h:31
@@ -35,1 +30,3 @@
+/// in the CFG. A branch statement decides which version is executed based on
+/// the runtime value of @p RTC.
 ///
----------------
We already wasted too much time on this, if you feel strong about this, leave it as it is.

However, I still think we should use a more descriptive name instead of RTC. To explain you this is not just me not _liking_ your names, I cite the LLVM developer policy:

"Avoid abbreviations unless they are well known"

http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

In LLVM there are a couple of uses of RT to abbreviate run time, and in fact RTCheck or RTCondition is a lot clearer to me. Maybe that works for you as well.




================
Comment at: include/polly/Support/ScopHelper.h:55
@@ -54,3 +54,3 @@
 
-/// @brief Simplify the region in a scop to have a single entry edge
-///        and a single exit edge.
+/// @brief Simplify the region in a scop to have a single uncondionaly entry
+///        edge and a single exit edge.
----------------
typo: unconditional

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:578
@@ +577,3 @@
+
+  /// @brief The loop anotator to generate llvm.loop metadata.
+  LoopAnnotator Annotator;
----------------
typo: annotator

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:581
@@ +580,3 @@
+  /// @brief The loop anotator to generate llvm.loop metadata.
+  LoopAnnotator Annotator;
+
----------------
jdoerfert wrote:
> grosser wrote:
> > By moving the LoopAnnotator into class scop, we extend its lifetime. This means the code generation of different scops will use the same loop annotator. Hence, in case an earlier scop leaves the loop annotator in non-clear state, this may impact later scops.
> > 
> > Was this intentional?
> Yes.
> 
> I'd say:
>   Don't leave anything in an unclean state and comment your variables like:
>     "LoobAnnotator Annotator;"
> 
> Is there any reason (except the state thing) that would benefit from constructing a new one all the time?
I don't think the overhead of (re)constructing it is measurable.

In terms of state, I was not talking about clean, but clear. Assuming, the LoopAnnotator
would contain a std::set, which would be _cleared_ on destruction, your change could cause old pointers to remain in this set unintended.

Keeping the live-time of variables short, avoids the need to think about such changes. In this case I looked into the LoopAnnotator and moving it seems to not break any assumptions.

http://reviews.llvm.org/D5076