[PATCH] [Polly][RTC] Bail if too many parameters are involved in an RTC access.

[Tobias Grosser]

Hi Johannes,

thanks for the patch. The code itself looks good, it has nice test cases and it fixes the
issue we see with linpack.

I have a couple of minor comments. Otherwise it LGTM.

Thanks again,
Tobias


> [Polly][RTC] Bail if too many parameters are involved in an RTC access.

in a RTC ...

> If two many parameters are involved in accesses used to create RTCs

If too many
  
> we might end up with enormous compile times and RTC expressions.
> The reason is that the lexmin/lexmax is dependent on all these
> parameters and isl might need to create a case for every "ordering"
> of them (e.g., p0 <= p1 <= p2, p1 <= p0 <= p2, ...).
> 
> The exact number of parameters allowed in accesses is defined by the
> command line option -polly-rtc-max-parameters=XXX and set by default
> to 8.

================
Comment at: lib/Analysis/ScopInfo.cpp:1146
@@ +1145,3 @@
+  // Restrict the number of parameters involved in the access as the lexmin/
+  // lexmax computation will take too long if this number is hight.
+  //
----------------
high.


================
Comment at: lib/Analysis/ScopInfo.cpp:1150
@@ +1149,3 @@
+  //
+  //  #Parameters invovled | Time (in sec)
+  //            6          |     0.01
----------------
involved

================
Comment at: lib/Analysis/ScopInfo.cpp:1158
@@ +1157,3 @@
+  //           12          |    30.38
+  //
+  if (isl_set_n_param(Set) > RunTimeChecksMaxParameters) {
----------------
Nice that you performed some benchmarking.

================
Comment at: lib/Analysis/ScopInfo.cpp:1347
@@ -1306,1 +1346,3 @@
+  isl_ctx_reset_operations(getIslCtx());
+  return Valid;
 }
----------------
Did you find a test case that causes troubles here?

In Polly we only use the compute out to guard the dependence check, so if we reset the
operations we should probably do this at the beginning of the dependence check, where
we set the max operations for the compute out as well. Outside of the dependence check,
we do not enforce a quota, so I do not really see how other passes could fail. If you have  a test case that fails here, then I probably missed something. Otherwise, I would probably leave this one out.

================
Comment at: lib/Analysis/ScopInfo.cpp:1704
@@ +1703,3 @@
+        << " could not be created as the number of parameters involved is too "
+           "hight. The SCoP will be "
+           "dismissed.\nUse:\n\t--polly-rtc-max-parameters=X\nto adjust the "
----------------
high

================
Comment at: lib/Analysis/ScopInfo.cpp:1707
@@ +1706,3 @@
+           "maximal number of parameters but be advised that the compile time "
+           "might increase exponentially.\n\n");
+
----------------
quadratically?

================
Comment at: test/ScopInfo/aliasing_many_parameters_not_all_involved.ll:4
@@ +3,3 @@
+;
+; Check that we allow this SCoP even though it has 10 parameters involved in posisbly aliasing accesses.
+; However, only 7 are involved in accesses through B, 8 through C and none in accesses through A.
----------------
possibly

Also, this line is longer than 80 columns, no?




================
Comment at: test/ScopInfo/aliasing_many_parameters_not_all_involved.ll:5
@@ +4,3 @@
+; Check that we allow this SCoP even though it has 10 parameters involved in posisbly aliasing accesses.
+; However, only 7 are involved in accesses through B, 8 through C and none in accesses through A.
+;
----------------
This is a really nice test case.

http://reviews.llvm.org/D5500