[PATCH] [Polly] Use ISL to Determine Loop Trip Count

[Johannes Doerfert]

Some comments are inlined. Except those and the test coverage we could improve the patch LGTM.

We should probably add a test case for the nsw problem, e.g., that we do not check for nsw and therefore can assume an unconstrainted loop. The example should probably look similar to the one below, however we would need to force ScalarEvolution to not recognize the loop trip count.

  // Infinite loop for N < 0 in a non-wrapping world.
  for (int i = 0; i < N; i++)
    A[i] = i;


================
Comment at: lib/Analysis/ScopDetection.cpp:708
@@ +707,3 @@
+  if (AllowNonSCEVBackedgeTakenCount && L->getNumBackEdges() == 1) {
+    auto *ExitingBlock = L->getExitingBlock();
+    if (ExitingBlock)
----------------
move this in the if of the next line 

================
Comment at: lib/Analysis/ScopDetection.cpp:969
@@ -948,1 +968,3 @@
+  if (!DetectRegionsWithoutLoops && !Context.hasAffineLoops &&
+      !AllowNonSCEVBackedgeTakenCount)
     invalid<ReportUnprofitable>(Context, /*Assert=*/true, &CurRegion);
----------------
I don't think we need this as the loops we now allow are "affine" hence the hasAffineLoops should be set to true if we encountered one. If not we want to bail even if we would allow such loops.

================
Comment at: lib/Analysis/ScopInfo.cpp:77
@@ -76,1 +76,3 @@
 
+static int getLoopDepth(const Scop *S, const Loop *L) {
+  Loop *outerLoop = S->getRegion().outermostLoopInRegion(const_cast<Loop *>(L));
----------------
I know there was no documentation for this function before, but maybe you could write a short one that explains that this computes not the real loop-depth (in the sence of Loop/LoopInfo) but a relative one in the SCoP.

================
Comment at: lib/Analysis/ScopInfo.cpp:742
@@ +741,3 @@
+
+  if (isl_map_dim(Map, isl_dim_in) == 0) {
+    isl_local_space_free(MapLocalSpace);
----------------
I don't get this change. If the input space (setDomain) was 0-dimensional we return a map like: [ ] -> [ ] ?

Could you give me some intuition why?

================
Comment at: lib/Analysis/ScopInfo.cpp:922
@@ +921,3 @@
+  isl_aff *LoopAff = isl_multi_aff_get_aff(LoopMAff, loopDimension);
+  LoopAff = isl_aff_add_constant_si(LoopAff, 1);
+  LoopMAff = isl_multi_aff_set_aff(LoopMAff, loopDimension, LoopAff);
----------------
Our virtual loops (i0,i1,..) always start at 0. We should keep it that way otherwise we will soon end up with an offset by 1 mess. This should also simplify this code block a bit.

================
Comment at: lib/Analysis/ScopInfo.cpp:933
@@ +932,3 @@
+  auto *Cond = dyn_cast<ICmpInst>(Term->getCondition());
+  assert(Cond && !Cond->isUnsigned() && "Condition is not signed");
+
----------------
We should not enforce unsigned conditions, they are/will be supported soon (at least under a flag).

================
Comment at: test/ScopInfo/isl_trip_count.ll:3
@@ +2,3 @@
+;
+; CHECK: [M, N] -> { Stmt_while_body[i0] : 4i0 <= -M + N and i0 >= 1; Stmt_while_body[0] }
+;
----------------
see offset-by-one comment above.

http://reviews.llvm.org/D9444

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/