[PATCH] D18450: [Polly] Exploit graph properties during domain generation

[Tobias Grosser via llvm-commits]

grosser accepted this revision.
grosser added a comment.
This revision is now accepted and ready to land.

Hi Johannes,

thanks a lot for splitting up this function and and commenting the new features. This is now indeed a lot clearer to me. I added a couple of smaller comments, but most are rather minor or typos.

>From my point you can commit these changes. When doing so, it would be great if you could split this into a couple of independent commits such that functional and non-functional changes are clear even to an outsider. These seem to be independent changes to me:

1. Pull out *adjustDomainDimensions [NFC]
2. Add addDomainDimId at some places (fixes michael's bug?)
3. Propagating constrains
4. Something that changes domains (unclear were)

Best,
Tobias


================
Comment at: include/polly/ScopInfo.h:1429
@@ +1428,3 @@
+  /// a block A dominates a block B and B post-dominates A we know that the
+  /// domain of B is a superset of the domain of A. As we do not have
+  /// post-dominator information available here we use the less precise region
----------------
Is this dominance/post-dominance property true if A is part of a loop, which does not contain B? Something like:

```
pre:
  br label %A

A:
  br %cnd, %A, %B

B:
```

The domains of A and B may have different dimensionality and can consequently not be be compared, right?

What about unreachables/infinite loops? Do they need to be considered? Assuming we commit the "fixed" the post-dominator patch, the exit of a region may be reachable by less parameters than the input?

Adding a short note what happens with these exceptions would be nice.


================
Comment at: lib/Analysis/ScopInfo.cpp:2195
@@ +2194,3 @@
+/// This function assumes @p NewL and @p OldL are equal or there is a CFG
+/// edge from @p OldL to @p NewL.
+static __isl_give isl_set *adjustDomainDimensions(Scop &S,
----------------
Pulling this out into a separate function is a very good idea. It makes the code a lot more readable and removes code duplication. Could this + ​getFirstNonBoxedLoopFor could be a separate [NFC]? In case it can, I would suggest to pull this out and commit it ahead of time. (no further review required).

================
Comment at: lib/Analysis/ScopInfo.cpp:2217
@@ +2216,3 @@
+  //      => Loops were left were difference of the depths defines how many.
+  if (OldDepth == NewDepth) {
+    Dom = isl_set_project_out(Dom, isl_dim_set, NewDepth, 1);
----------------
Maybe also add an assert(NewL->getParentLoop() == OldL->getParentLoop()

================
Comment at: lib/Analysis/ScopInfo.cpp:2225
@@ +2224,3 @@
+    Dom = isl_set_add_dims(Dom, isl_dim_set, 1);
+    Dom = addDomainDimId(Dom, NewDepth, NewL);
+  } else {
----------------
You seem to add here 'addDomainDimId', which was not here before. Do I remember correctly that this fixes Michael's bug? In case it does, I suggest to commit just this line as independent functional change together with Michael's test case and include a commit message that explains _why_ this became necessary.

================
Comment at: lib/Analysis/ScopInfo.cpp:2234
@@ +2233,3 @@
+
+  return Dom;
+}
----------------
Overall, this function is a lot more readable then the old version. Very nice!

================
Comment at: lib/Analysis/ScopInfo.cpp:2256
@@ +2255,3 @@
+
+  // Since the dimensions of @p BB and @p ExitBB might be differen we have to
+  // adjust the domain before we can propagate it.
----------------
different

================
Comment at: lib/Analysis/ScopInfo.cpp:2319
@@ +2318,3 @@
+
+    // If all successors of BB have been set a domain though the propagation
+    // above we do not need to build condition sets but can just skip this
----------------
through

================
Comment at: lib/Analysis/ScopInfo.cpp:2349
@@ -2245,1 +2348,3 @@
 
+      // If we propagates the domain of some block to "SuccBB" we do not have to
+      // adjust the domain.
----------------
propagate (without the 's')

================
Comment at: lib/Analysis/ScopInfo.cpp:2412
@@ +2411,3 @@
+                                               LoopInfo &LI) {
+  // If @p BB is the ScopEntry we are done
+  if (R.getEntry() == BB)
----------------
Point at end of sentence.

================
Comment at: lib/Analysis/ScopInfo.cpp:2429
@@ +2428,3 @@
+  // Set of regions of which the entry block domain has been propagated to BB.
+  // all predecessor inside any of the regions can be skipped.
+  SmallSet<Region *, 8> PropagatedRegions;
----------------
All predecessors

================
Comment at: lib/Analysis/ScopInfo.cpp:2433
@@ +2432,3 @@
+  for (auto *PredBB : predecessors(BB)) {
+    // Skip backedges
+    if (DT.dominates(BB, PredBB))
----------------
Point.

================
Comment at: test/ScopInfo/complex-successor-structure-2.ll:10
@@ +9,3 @@
+; the domains simple. We will bail anyway due to invalid required invariant
+; loads.
+;
----------------
Nice!

================
Comment at: test/ScopInfo/complex-successor-structure-3.ll:15
@@ +14,3 @@
+; CHECK-NEXT:            Schedule :=
+; CHECK-NEXT:                [tmp5, tmp, tmp8, tmp11, tmp14, tmp17, tmp20, tmp23, tmp26, p_9, p_10, p_11, p_12] -> { Stmt_FINAL[] -> [22] };
+;
----------------
Nice.

================
Comment at: test/ScopInfo/complex-successor-structure.ll:4
@@ -3,4 +3,3 @@
 
-; We build scops from a region of for.body->B13 having successor nodes
-; of following form and check that the domain construction does not take a huge
-; amount of time.
+; We build a scop from the region for.body->B13. The CFG has is of the
+; following form. The test checks that the condition construction does not take
----------------
has is -> drop one of them.


http://reviews.llvm.org/D18450