[PATCH] D18450: Use RegionInfo as shortcut in part of the domain generation [WIP]

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

the idea of using Regioninfo to 'jump' over the domain propagation is great. I earlier tried to hint to using (post)-dom information, but using region information makes indeed more sense especially as post-dominator information is not (yet) useful in this context.

I just checked out this patch to try it out, but unfortunately I get timeouts for some test cases. I used the command 'arc patch http://reviews.llvm.org/D18450' to checkout the patch, which applied it on top of https://llvm.org/svn/llvm-project/polly/trunk@264286. I then rebased this patch to trunk (264514) where all test pass without timeout. Please let me know if I should have tried this patch with a different version of Polly.

This patch is not coming with any test cases and also does not explicitly describe the benefits it is intended to bring. My understanding is that this patch should both result in simpler domain constraints for certain test cases and should even allow us to construct domains for codes that could not been modeled previously, due to overly complex constraints being constructed. Could you point out these benefits in the commit message and add a test case that shows this benefit? I would expect test/ScopInfo/complex-successor-structure.ll to benefit. However, when I tried it no scop is constructed. Do you understand why?


================
Comment at: lib/Analysis/ScopInfo.cpp:2229
@@ +2228,3 @@
+    // Check if we can use the region info to determine the domain of some
+    // other block from the domain of this one.
+    auto *BBReg = RI ? RI->getRegionFor(BB) : nullptr;
----------------
A higher level comment (possibly even with some example) would be very helpful to get a quick understanding of this code. In case this is the missing comment you are talking about in the "WIP-message".

I think I understood the code without this comment, but I would still like to review this patch with your comment in place to ensure what I understood is indeed what you aimed for.

================
Comment at: lib/Analysis/ScopInfo.cpp:2238
@@ +2237,3 @@
+        else
+          ExitDomain = isl_set_copy(Domain);
+        FinishedExitBlocks.insert(ExitBB);
----------------
The reason why  we have two branches would e.g. be interesting to point out in the comment.

================
Comment at: lib/Analysis/ScopInfo.cpp:2241
@@ +2240,3 @@
+      }
+    }
+
----------------
The overall size of this function grows after this patch to now over three screen pages. I think in general we should really try to split this function in smaller entities. To make sure that this patch is not adding even more to this growth, it might be worth to move the code above in its own function.

================
Comment at: lib/Analysis/ScopInfo.cpp:2331
@@ -2302,3 +2330,3 @@
   return true;
 }
 
----------------
This function has now grown to over three screen pages and is becoming increasingly difficult to read. Would there be an easy way to split this function into smaller pieces, possibly even before this commit is going in?


http://reviews.llvm.org/D18450