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

[Johannes Doerfert via llvm-commits]

jdoerfert added inline comments.

================
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
----------------
grosser wrote:
> 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.
> 
> Is this dominance/post-dominance property true if A is part of a loop, which does not contain B? Something like:
> The domains of A and B may have different dimensionality and can consequently not be be compared, right?

Short answer is Yes. Long answer:
Even now (with regions) we allow this and use the "adjustDimensionality" function to fix the dimensions of %pre to match %B in your example. Between %A and %B we need to consider %cnd anyway.

> 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?

Yes and no. While they are reachable by less inputs we do not model them that way. At least not until we allow "return" statements. If we encounter "unreachable" we will (and do) assume it to be not executed but that does not affect the domains of later notes. This is a problem (e.g. bug PR26683) but it also helps to keep domains concise. In any case, we can handle that differently here if we decide to do so.

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

Since nothing changed I can write what happens now, thus before and after the patch. Though, that is again something that is not related for me...



================
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,
----------------
grosser wrote:
> 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).
This sounds like unnecessary work but I will check if it makes sense.

================
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);
----------------
grosser wrote:
> Maybe also add an assert(NewL->getParentLoop() == OldL->getParentLoop()
Makes sense.

================
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 {
----------------
grosser wrote:
> 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.
1) It does not fix any bugs that I am aware of. If there was a bug caused by this it would crash at compile time.
2) I am not sure what Michaels bug was, this should not be it anyway.
3) This was and is necessary because the domains need to match when we combine them. That has __not__ changed with this commit.


http://reviews.llvm.org/D18450