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

[Michael Kruse via llvm-commits]

Meinersbur added a comment.

In http://reviews.llvm.org/D13341#258594, @jdoerfert wrote:

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


For DeLICM, while adding MemoryAccesses, there will be a multiple per-Stmt data for which I'd introduce new dictionaries (and lookups) from BasicBlocks to that data. We save this by just using the already existing StmtMap. Problem was, those was created only later.

In http://reviews.llvm.org/D13341#258594, @jdoerfert wrote:

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


This is an important point (but not the reason for this patch). Doing multiple unrelated things in one function makes code hard to understand and modify (e.g. changing the order of the two operations).


================
Comment at: include/polly/ScopInfo.h:1214
@@ -1213,1 +1213,3 @@
+  /// @see isIgnored()
+  void simplifySCoP(bool RemoveIgnoredStmts);
 
----------------
jdoerfert wrote:
> 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?
> What is a no-op statement here? 

One without side-effect. Currently it checks whether there is no MemoryAccess at all but we could also remove those which have no write access.


> And why should we build ignored statements in the first place? 

Because we don't know yet whether they are ignored. E.g. because of load hoisting.


> Aren't they deleted anyway? 

At the last simplifyScop, yes; but we don't need to do the calculations in-between when we already know they are going to be removed.  


> Do you have performance numbers for this patch by any chance?

No. I don't expect a noticeable impact on performance.

================
Comment at: lib/Analysis/ScopInfo.cpp:2312
@@ -2315,1 +2311,3 @@
+  // Remove statments without accesses.
+  simplifySCoP(false);
 
----------------
jdoerfert wrote:
> Calling simplifySCoP 3 times seems a lot. Alone in this method we (directly or indirectly) iterate 3 __more__ times over all statements now.
I removed one of them in the actual commit. Iterating over all statements is cheap.

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

================
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);
----------------
jdoerfert wrote:
> 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.
I think it was whole-scop-non-affine-subregion-in-loop.ll

This is by design. The affine region is removed because there are no accesses in it. That leaves no statements in the scop. How would you handle this?


Repository:
  rL LLVM

http://reviews.llvm.org/D13341