[PATCH] [Refactor] Cleanup runtime code generation

[Tobias Grosser]

Hi Johannes,

thanks a lot for putting the time to first improve the existing code before you add new features. I think this is very valuable in ensuring the code remains maintainable in the long run.

Regarding this patch, it seems the main contribution is that you factor most code into smaller helper functions. I think this is a good idea, as it makes the runOnScop() function a lot more readable.

It also seems your refactoring changes the way the run-time code is generated. Before we first built the run-time condition with a placeholder (i1 true) and then later replaced this placeholder with the actual run-time condition. Your new code now first generates the condition and then uses the result when introducing the run-time check. This change also has some semantic implications. Specifically, the code that evaluates the run-time condition is now inserted earlier. For the attached test case ({F160830}), this means evaluate the run-time condition even in cases, in which the actual scop is never executed. This seems to be a regression compared to the old code, right? Do you think we could get the same more readable code, even without these semantic changes?

I have a couple of more-detailed inline comments.

Cheers,
Tobias

> [Refactor] Cleanup runtime code generation

What is "runtime code generation"? I think this abbreviation is misleading.

> + Refactor the runtime condition build function
> + Use regexp in two test case.

I think this commit message is rather short. If you could explain in two or three cases what kind of changes you did this would help both the people who skim through the commit messages and also people like me who want to understand the patch. Things I asked myself:

- What are the actual refactoring changes that have been applied. Why are they beneficial?
- Are there any semantic changes?

E.g.:

 - Factor out code into helper functions to make runOnScop() function more readable. 
 - Change construction of run-time-check. Instead of first introducing a placeholder value that is later
   replaced  by the actual condition, we first build the condition and then use it directly in the run-time 
   check.
 - Make analysis pass variables class variables. (Why is this useful?)

================
Comment at: include/polly/CodeGen/IRBuilder.h:112
@@ -104,1 +111,3 @@
+  return Builder;
+}
 }
----------------
This looks good.

================
Comment at: include/polly/CodeGen/Utils.h:35
@@ -34,3 +30,4 @@
-/// compile time.
+/// in the CFG. A branch statement decides which version is executed based on
+/// the runtime value of @p RTC.
 ///
 /// Before transformation:
----------------
This comment was really outdated. It is good that we replace it.

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:578
@@ +577,3 @@
+  ScalarEvolution *SE;
+  ///}
+
----------------
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)?

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

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:593
@@ +592,3 @@
+    Value *RTC = ExprBuilder.create(AI->getRunCondition());
+    return Builder.CreateIsNotNull(RTC);
+  }
----------------
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.

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:616
@@ -604,2 +615,3 @@
 
-    NodeBuilder.create(Ast);
+    NodeBuilder.create(AI->getAst());
+
----------------
Very nice cleanup. This function is a lot more readable now.

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:618
@@ -606,1 +617,3 @@
+
+    DEBUG(StartBlock->getParent()->dump());
     return true;
----------------
Was adding this DEBUG statement intentional? I think it may be a little verbose for day-to-day use.

================
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"
----------------
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)

================
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) {
----------------
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.

================
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
 
----------------
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.

http://reviews.llvm.org/D5076