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

[Tobias Grosser via llvm-commits]

grosser 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
----------------
jdoerfert wrote:
> 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...
> 
> 
My understanding was that this comment was describing the shortcut of propagating constraints across regions and specifically justifying 
why it is safe to do so. As this optimization has not been present before the patch, I wonder how this can already happen today.

================
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,
----------------
jdoerfert wrote:
> 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.
It is currently still unclear to me which code changes introduce the precise
test cases changes. Splitting off functionality in parts that change output and changes that do not impact behavior is useful both for bisecting,  but especially to document which parts are non-functional-changes.

It also speed ups patch reviews immensely. In fact, as already said earlier, I very much invite you to commit clearly beneficial refactoring that are NFC directly only relying on post-commit review, such that the actual functional change that needs to be reviewed is smaller.

================
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 {
----------------
jdoerfert wrote:
> 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.
Thanks for clarifying.

Regarding 3): I understand that the calls to addDomainDimId have been already in buildDomainsWithBranchConstraints. However, in propagateDomainConstraints you replace very similar code with this function. The original code in  propagateDomainConstraints did not have these calls, so they seem to be added. Or did I miss something here? In case they are indeed new, it would be good to explicitly state in the commit that this change change does not change functionality.


http://reviews.llvm.org/D18450