[PATCH] D13341: [Polly] Earlier creation of ScopStmt objects

[Johannes Doerfert via llvm-commits]

jdoerfert added a comment.

Why don't we create statements (and accesses) during the domain generation [second part, propagation]?

Pro:

- We iterate over the SCoP anyway.
- We already know which blocks/regions we will keep (at least until hoisting) and which we ignore.
- The block/region can be checked for accesses at that point in time as good as at any prior one.
- The Domain for the new statement was just computed and can be used right away, thus:
  - No delayed `init()` calls needed.
  - No broken/useless ScopStmts flying around.
  - No need to remove anything we can easily avoid building.

Con:

- Domain generation would perform another task (though, so far schedule generation did this and it seemed fine).


================
Comment at: include/polly/ScopInfo.h:1214
@@ -1213,1 +1213,3 @@
+  /// @see isIgnored()
+  void simplifySCoP(bool RemoveIgnoredStmts);
 
----------------
What is a no-op statement here? And why should we build ignored statements in the first place? Aren't they deleted anyway? Do you have performance numbers for this patch by any chance?

================
Comment at: lib/Analysis/ScopInfo.cpp:1154
@@ +1153,3 @@
+  } else {
+    for (BasicBlock *Block : R->blocks()) {
+      deriveAssumptions(Block);
----------------
braces are not needed.

================
Comment at: lib/Analysis/ScopInfo.cpp:2312
@@ -2315,1 +2311,3 @@
+  // Remove statments without accesses.
+  simplifySCoP(false);
 
----------------
Calling simplifySCoP 3 times seems a lot. Alone in this method we (directly or indirectly) iterate 3 __more__ times over all statements now.

================
Comment at: lib/Analysis/ScopInfo.cpp:2380
@@ +2379,3 @@
+
+    if (StmtIt->isEmpty() || (RemoveIgnoredStmts && isIgnored(RN))) {
+      // Remove the statement because it is unnecessary.
----------------
Please try to keep/write early exit code instead of indention.

================
Comment at: lib/Analysis/ScopInfo.cpp:2898
@@ +2897,3 @@
+      Domain = isl_set_empty(isl_space_set_alloc(IslCtx, 0, 0));
+    }
+    auto *UDomain = isl_union_set_from_set(Domain);
----------------
Meinersbur wrote:
> A test case fails without it. We might instead bail out early as invalid when we encounter a Scop without statements.
Which test? Unit or lnt? Shouldn't we fix this somehow? Generating an empyt domain for the case there is no statement seems rather odd.


Repository:
  rL LLVM

http://reviews.llvm.org/D13341