[PATCH] D63304: Ignore Singletons in statement domains

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Thank you for applying most of my suggestions.



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


================
Comment at: lib/Analysis/ScopInfo.cpp:2905
+    isl::set DimOnly = Set;
+    if (i > 0) // Remove left dimensions.
+      DimOnly = DimOnly.project_out(isl::dim::set, 0, i);
----------------
Condition is unnecessay: `.project_out(isl::dim::set, 0, 0)` will just return the original set.


================
Comment at: lib/Analysis/ScopInfo.cpp:3007
+    for (isl::basic_set BSet : Domain.get_basic_set_list()) {
+      isl::set Set(BSet);
+      // Ignore the set that has only one iteration.
----------------
A `basic_set` should convert implicitly to a `set` when passed to `hasSingleton`


================
Comment at: lib/Analysis/ScopInfo.cpp:3013
+      }
+      UpperBoundParams = UpperBoundParams.unite(BSet.lexmax().params());
+    }
----------------
[serious] I am quite sure that this method removing the single-iteration-domain is not reliable. Examples include:
```
for (int i = 0; i < -n; i+=1)
  Stmt(i);
```
```
if (unsigned i = 0; i < (n%8); i+=1)
  Stmt(i);
```
and combinations thereof. At least the first would do the exact opposite: Remove all contexts that have more than one iteration.


================
Comment at: lib/Analysis/ScopInfo.cpp:3022
+  }
+  if (!RestrictDomainParams.plain_is_universe()) {
+    for (ScopStmt &Stmt : *this) {
----------------
Is this a safeguard to avoid excluding everything? Could you write a comment about its intention?


================
Comment at: lib/Analysis/ScopInfo.cpp:3035
+void Scop::enforceAssumptionsForAvoidsingleIter() {
+  AssumedContext = AssumedContext.intersect(getRestrictDomainParams());
+}
----------------
Could you explain why `RestrictDomainParams` has to be collected separately from `AssumedContext` and only to be merged afterwards?


================
Comment at: test/ScopDetect/model-dowhile.ll:2
+; REQUIRES: asserts
+; RUN: opt %loadPolly -S -O3 -debug-only=polly-ast -debug-only=polly-scops -polly-ast -polly-avoid-singular-loops=false --disable-output %s |& FileCheck %s --check-prefix=NOMODEL
+; RUN: opt %loadPolly -S -O3 -debug-only=polly-ast -debug-only=polly-scops -polly-ast -polly-avoid-singular-loops=true --disable-output  %s |& FileCheck %s --check-prefix=MODEL
----------------
The `-S` flag is unnecessary with `--disable-output`


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


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

https://reviews.llvm.org/D63304