[PATCH] [Refactor] Cleanup runtime code generation

[Johannes Doerfert]

I will again look into the changed RTC location, namely where we generate the RTC instructions. I don't think it makes a difference but I will change it back to split_new_and_old if possible, or at least try to argue why we don't need that (LLVM optimizations do similar stuff all the time).

I also added a few inline comments because I do not agree with some of the review.

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:578
@@ +577,3 @@
+  ScalarEvolution *SE;
+  ///}
+
----------------
grosser wrote:
> Making these analysis pass definitions class variables seems a conceptually independent change. It possibly does not hurt in this patch, but if you believe tracking those analysis passes as class variables is better style, I wonder if we should not have an independent patch that does this kind of transformation all over Polly. This patch would help to educate other people about the reasons this style is preferred and could also be a reference in future patch reviews.
> 
> There are other similar uses in ScheduleOptimizer.cpp, ScopInfo.cpp.
> 
> Also, before writing such a patch, it may be good to discuss the reason why this change is preferred. I personally try to always make the life time of a variable as short as possible. This change goes against this goal. So it would be good to understand why you believe this is better? There may be very good reasons, which I may have missed. Is this e.g. to align our style with LLVM? Or just to have a more consistent style in Polly (our code is rather inconsistent)?
A few thoughts to this commit (not all related to each other):

  - Why do I need to extract non invasiv move of 4 variables in a refactoring patch? At some point the amount of work I'm supposed to put into "cleaning" my patches becomes ridiculous, especially compared to what else goes in.
  - Polly is inconsistent here, we have both styles, local variables and class members, however the latter can "always" be used.
  - The class I edit here has one function,...one. Where these variables are defined (in this one function) or as a memeber will only change the kind of comment we can add to their declaration.



================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:581
@@ +580,3 @@
+  /// @brief The loop anotator to generate llvm.loop metadata.
+  LoopAnnotator Annotator;
+
----------------
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?

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:593
@@ +592,3 @@
+    Value *RTC = ExprBuilder.create(AI->getRunCondition());
+    return Builder.CreateIsNotNull(RTC);
+  }
----------------
grosser wrote:
> Creating a new function for run time condition handling makes this code a lot more readable.
> 
> Two minor remarks:
> 
>  - Please use RunTimeCondition instead of RTC, as this is not really a common expression.
>  - Run time conditions can conceptually be used (and hopefully will be used) for a lot more than
>    delinearization. Hence, calling it delinearization condition is misleading. However, we could
>    still use delinearization as an example or what a run time condition is or where it is used.
  - You don't like my variables names (even though they are clear abrivations of the full names) and now I have to change that?

  - At the moment there is only delinearization, why should I paint a picture of the future in the description of a feature?

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:618
@@ -606,1 +617,3 @@
+
+    DEBUG(StartBlock->getParent()->dump());
     return true;
----------------
grosser wrote:
> Was adding this DEBUG statement intentional? I think it may be a little verbose for day-to-day use.
I like to see the endresult of the code generation if I debug the whole thing, if I'm the only one then I can remove the stmt.

================
Comment at: test/Isl/CodeGen/blas_sscal_simplified.ll:8
@@ +7,3 @@
+; }
+;
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
----------------
grosser wrote:
> It is unclear what is tested in this test case. I assume this is the test case that broke previously. Could you add a comment to it to explain why it failed before. Something like.
> 
> // This test case segfaulted previously due to us not properly supporting load instructions followed by zexts in the
> // run-time condition.
> 
> (Please replace by the actual problem)
I will add something.

================
Comment at: test/Isl/CodeGen/multidim_2d_parametric_array_static_loop_bounds.ll:17
@@ -18,2 +16,3 @@
+; CHECK: br i1 %[[T2]], label %polly.start, label %for.i
 
 define void @foo(i64 %n, i64 %m, double* %A) {
----------------
grosser wrote:
> This test case explicitly tests that the computations for the run-time condition are created in the basic block that is named polly.split_new_and_old, the block in which we branch according to the run-time condition. This seems a good place to put those instructions.
> 
> It seems your patch changes this to now generate the instructions somewhere earlier. Where exactly?  Was this intentional? In case it was, it would be good to explain the motivation/reason behind this.
> 
> Also, any changes to the generated IR seems suspicious in re-factorings that to my understanding aim to not change functionality.
The first part (the BB where the RTC is generated) is a valid comment. I will look into that anyway but I think we can generate it again in split_new_and_old or we can argue LLVM will move it there anyway.
However, the second part (about the IR change) is not helping at all. If you test for __unnamed__ variables every little thing can change the result, even stupid stuff like the ordering of the basic blocks in the function. This would clearly be without functionality change but it could trigger your test...

================
Comment at: test/Isl/CodeGen/run-time-condition-with-scev-parameters.ll:8
@@ -8,1 +7,3 @@
+; CHECK: %[[T2:[._a-zA-Z0-9]]] = select i1 %[[T1]], i64 1, i64 0
+; CHECK: %[[T3:[._a-zA-Z0-9]]] = icmp ne i64 %[[T2]], 0
 
----------------
grosser wrote:
> I just checked what has changed in the actual test case output and it seems the actual change is:
> 
> -; CHECK: %4 = icmp ne i64 0, %3
> +; CHECK: %4 = icmp ne i64 %3, 0
> 
> Introducing the regexp seems not necessary here and also hides the actual change needed. In fact, I run this test case trying to understand why your refactoring caused the statement numbers to change.
Even if it doesn't change the statement numbers, someone at somepoint will and he/she/it has to fix test cases like this with no good reason to hardcode these numbers. We do not care if it is called %1 or %0 here, why test for it?!?

http://reviews.llvm.org/D5076