[PATCH] D12428: Traverse the SCoP to compute non-loop-carried domain conditions

[Johannes Doerfert via llvm-commits]

jdoerfert added a comment.

Some comments for the new version.


================
Comment at: include/polly/ScopInfo.h:900
@@ -897,1 +899,3 @@
+  Scop(Region &R, ScalarEvolution &SE, DominatorTree &DT, isl_ctx *ctx,
+       unsigned MaxLoopDepth);
 
----------------
grosser wrote:
> @params
A different commit should introduce these.

================
Comment at: lib/Analysis/ScopInfo.cpp:779
@@ +778,3 @@
+                   SmallVectorImpl<__isl_give isl_set *> &ConditionSets) {
+
+  if (BI->isUnconditional()) {
----------------
grosser wrote:
> You can probably derive  the Loop L from BI itself.
Not without the loop info and I already got the loop anyway.

================
Comment at: lib/Analysis/ScopInfo.cpp:787
@@ +786,3 @@
+
+  isl_set *ConsequenceCondSet = nullptr;
+  if (auto *CCond = dyn_cast<ConstantInt>(Condition)) {
----------------
grosser wrote:
> What about ThenCondSet, ElseCondSet?
What about them? I don't think the names are better (as they give the wrong intuation about the conditional here) but I don't care much.

================
Comment at: lib/Analysis/ScopInfo.cpp:796
@@ +795,3 @@
+    assert(ICond &&
+           "Condition of exiting branch was neither constant nor ICmp!");
+
----------------
grosser wrote:
> I would personally write this as
> 
> } else (auto *ICond = dyn_cast<ICmpInst>(Condition)) {
> 
> } else {
>   llvm_unreachable("Condition of exiting branch was neither constant nor ICmp!");
> }
That gives me another else case and a hard to parse middle if. As anything that is not constant or icmp is broken anyway and should never never happen I don't see the need to complicate the code.

================
Comment at: lib/Analysis/ScopInfo.cpp:1497
@@ +1496,3 @@
+  while (L && !L->contains(EntryBB))
+    L = L->getParentLoop();
+  if (L)
----------------
grosser wrote:
> You are looking for the innermost loop that contains the scop but is itself not fully contained in the scop, right? This function is similar to what I proposed in the discussion of Pratik's patch. I think it would fit nicely into RegionInfo next to RegionInfo::outermostLoopInRegion().
> 
> To compute this loop, I do not see why you need to look at the predecessors of EntryBB. Are the following three lines not enough?
> 
> ```
> Loop L = LI->getLoopFor(R->getEntry)
> while (L && R->contains(L)) 
> L = L.getParentLoop()
> ```
> 
> This can probably be committed without pre-commit review.
I use the getRelativeLoopDepth() now. That does the trick, hence this code is gone.

================
Comment at: lib/Analysis/ScopInfo.cpp:1502
@@ +1501,3 @@
+  auto *S = isl_set_universe(isl_space_set_alloc(getIslCtx(), 0, MaxLoopDepth));
+  DomainMap[R->getEntry()] = S;
+
----------------
grosser wrote:
> I would personally prefer to start with a zero-dimensional set and then add dimensions as necessary, rather than dropping dimensions later on. However, this is probably not relevant for correctness.
I think this makes the code simpler, so I'd suggest we change it at some later point. OK?

================
Comment at: lib/Analysis/ScopInfo.cpp:1506
@@ +1505,3 @@
+}
+
+void Scop::addBranchConstraintsToDomain(Region *R, unsigned PreScopLoopDepth,
----------------
grosser wrote:
> This algorithm is a lot more elegant as what we had before. I especially like the use of the ReversePostOrderTraversal.
> 
> It would be nice to add 5-10 lines that give an overview that describe the algorithm a little. E.g. how is the CFG walked (how does a Reverse PostOrder look like) and why it is needed for the correctness of the algorithm.
> How is the information propagated through the graph? Maybe even a little ASCII art?
> 
> 
> 
> 
> Maybe even a little ASCII art?

Michael spoiled you...

================
Comment at: lib/Analysis/ScopInfo.cpp:1543
@@ +1542,3 @@
+
+      Loop *SuccBBLoop = LI.getLoopFor(SuccBB);
+      if (BBLoop && SuccBBLoop != BBLoop &&
----------------
grosser wrote:
> This part of the code also needs a comment (or needs to be described in the function-header comment).
> 
> To my understanding you check if a block is a loop exit block and in this case you drop the constraints on the additional dimensions. Alternatively, you could just use the conditions that hold at the loop entry, right? Would
> this allow us to get rid of the dimension dropping all together?
> To my understanding you check if a block is a loop exit block and in this case you drop the constraints on the additional dimensions.
Yes.

> Alternatively, you could just use the conditions that hold at the loop entry, right? 
I am not sure which loop entry you refere to but I don't think that you can.

> Would this allow us to get rid of the dimension dropping all together?
I don't think that you can get rid of it.


http://reviews.llvm.org/D12428