[PATCH] D63304: Ignore Singletons in statement domains

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Thanks for the patch. Do you happen to have performance, code size numbers?

[suggestion] The algorithm for detecting non-looping basic sets is quite ISL-heavy. If the goal is to better handle loops such as

  i = 0;
  do {
    Stmt(i)
  } while (++i <n)

did you think about recognizing them directly? 
Another possibility would be to merge the singleton with the main loop. E.g. the loop above could be converted into

  for (int i = 0; i < max(1,n); i+=1) 
    Stmt(i);



================
Comment at: include/polly/ScopInfo.h:69
   DELINEARIZATION,
+  UPPERBOUND,
 };
----------------
Can you use something more descriptive?


================
Comment at: lib/Analysis/ScopInfo.cpp:195
+static cl::opt<bool>
+    HandleDoWhile("polly-model-do-while",
+                  cl::desc("Prune exeution domain of statements to ignore"
----------------
Can you explain the why you chose the name `model-do-while`?


================
Comment at: lib/Analysis/ScopInfo.cpp:1215
 
+void ScopStmt::updateDomain(isl::set NewDomain) { Domain = NewDomain; }
+
----------------
`setDomain` would better describe what this function does.


================
Comment at: lib/Analysis/ScopInfo.cpp:2182
   AssumptionContext = AssumptionContext.gist_params(S.getContext());
+  if (HandleDoWhile && !S.getRestrictDomainParams().plain_is_universe())
+    AssumptionContext =
----------------
No need to check `plain_is_universe()`; intersection with the universe would not change the set. ISL also has a fast-path for plain_universe arguments.


================
Comment at: lib/Analysis/ScopInfo.cpp:2183-2184
+  if (HandleDoWhile && !S.getRestrictDomainParams().plain_is_universe())
+    AssumptionContext =
+        AssumptionContext.intersect(S.getRestrictDomainParams());
   return AssumptionContext;
----------------
[serious] `simplifyAssumptionContext` should not logically modify the `AssumptionContext`. The intersection should have happened earlier.


================
Comment at: lib/Analysis/ScopInfo.cpp:2896
 
+static bool hasSingleton(isl::set Set) {
+  if (Set.is_singleton())
----------------
Please document/doxygen what this function is doing


================
Comment at: lib/Analysis/ScopInfo.cpp:2905
+      DimOnly = DimOnly.project_out(isl::dim::set, 0, i);
+    if (Dims - i - 1 > 0) // Remove right dimensions.
+      DimOnly = DimOnly.project_out(isl::dim::set, 1, Dims - i - 1);
----------------
Condition not necessary: project_out will not do anything when projecting-out zero dimensions.


================
Comment at: lib/Analysis/ScopInfo.cpp:2996
+      }
+      UpperBoundParams = UpperBoundParams.unite(BSet.lexmax().params());
+    }
----------------
[serious] Instead of `lexmax` on the scatter function, shouldn't we subtract the parameter conditions that cause the singleton set to be executed? For instance, I imagine this would break the lexmax logic (smaller `n` means more loop iterations):
```
for (int i = 0; i < -n; i+=1)
  Stmt(i);
```

What is the reason for this logic to be in `addLoopBoundsToHeaderDomain` and not e.g. in a separate function like `intersectBBDomainParams`? I prefer having the logic handling this in one place.


================
Comment at: lib/Analysis/ScopInfo.cpp:3017
 
+void Scop::intersectBBDomainParams() {
+  if (!HandleDoWhile)
----------------
We are trying to move the functionality that is only required for the construction of a SCoP into ScopBuilder.


================
Comment at: lib/Analysis/ScopInfo.cpp:3748
+  case UPPERBOUND:
+    return "Trip Count Upper Bound";
   }
----------------
[suggestion] A different description such as "Avoid single iteration loops?


================
Comment at: test/ScopDetect/model-dowhile.ll:1
+; REQUIRES: asserts
+
----------------
[serious] Please move `RUN` lines to the beginning


================
Comment at: test/ScopDetect/model-dowhile.ll:28
+
+; RUN: opt -S -polly -O3 -debug-only=polly-ast -debug-only=polly-scops %s -polly-model-do-while=false &> %t1
+; RUN: opt -S -polly -O3 -debug-only=polly-ast -debug-only=polly-scops %s -polly-model-do-while=true &> %t2
----------------
[serious] Use `%loadPolly`

To avoid mixing stdout and stderr (how they are interleaved is undefined), you can use `--disable-output`


================
Comment at: test/ScopDetect/model-dowhile.ll:31
+
+; RUN: FileCheck %s < %t1 --check-prefix=NOMODEL
+; RUN: FileCheck %s < %t2 --check-prefix=MODEL
----------------
[serious] Use `|` to directly pipe to FileCheck


================
Comment at: test/ScopDetect/model-dowhile.ll:73
+
+; <label>:6:                                      ; preds = %4
+  %7 = bitcast [80 x [80 x [80 x i32]]]* %5 to i8*
----------------
[serious] Please unsure that every value has a name (anon values tend to cause less table output). You can do this using `opt -instnamer`.


================
Comment at: test/ScopDetect/model-dowhile.ll:107
+; <label>:25:                                     ; preds = %22
+  call void @llvm.lifetime.end.p0i8(i64 2048000, i8* nonnull %7) #2
+  ret void
----------------
Remove these markers? They should be irrelevant for this test case.


================
Comment at: test/ScopDetect/model-dowhile.ll:117-118
+
+attributes #1 = { argmemonly nounwind }
+attributes #2 = { nounwind }
----------------
[nit] attributes are unused. 


Repository:
  rPLO Polly

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63304/new/

https://reviews.llvm.org/D63304