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

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

nice patch. This is a great first step to move towards removing TempScopInfo. The algorithm you use also seems very elegant. I have just one conceptual questions:

For loop exit nodes, would it be possible to just use the domains/constraints from the LoopEntry node rather than using the constraints of the exiting node and to than drop constraints referring to inner loop dimensions. I am asking for two reasons: 1) Dropping constraints probably works in this case, but always feels a little shady as isl could e.g. drop constraints on outer loop iterators if already implied by the constraints on the inner iterators. Now if we drop the constraints on the inner iterators, we may loose the constraints on the outer iterators as well, 2) I somehow prefer to keep the number of dimensions in the domain-sets to exactly the number of dimensions needed per basic-block. Using the maximal size and then dropping dimensions does not seem nice and possibly also has a (minor?) compile time cost.

Otherwise, the patch looks fine. One larger comment describing your algorithm would be nice though.

I also added some minor stylistic remarks.

Best,
Tobias


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

================
Comment at: include/polly/ScopInfo.h:913
@@ +912,3 @@
+
+  /// @brief Compute the branching constraints for each basic block in @p R.
+  void addBranchConstraintsToDomain(Region *R, unsigned PreScopLoopDepth,
----------------
@params?

================
Comment at: include/polly/ScopInfo.h:920
@@ -907,1 +919,3 @@
+  void buildDomains(Region *R, LoopInfo &LI, ScopDetection &SD,
+                    DominatorTree &DT);
 
----------------
@params

================
Comment at: include/polly/ScopInfo.h:1205
@@ +1204,3 @@
+  /// @brief Return the non-loop carried conditions on the domain of @p Stmt.
+  __isl_give isl_set *getDomainConditions(ScopStmt *Stmt);
+
----------------
@params

================
Comment at: lib/Analysis/ScopInfo.cpp:779
@@ +778,3 @@
+                   SmallVectorImpl<__isl_give isl_set *> &ConditionSets) {
+
+  if (BI->isUnconditional()) {
----------------
You can probably derive  the Loop L from BI itself.

================
Comment at: lib/Analysis/ScopInfo.cpp:787
@@ +786,3 @@
+
+  isl_set *ConsequenceCondSet = nullptr;
+  if (auto *CCond = dyn_cast<ConstantInt>(Condition)) {
----------------
What about ThenCondSet, ElseCondSet?

================
Comment at: lib/Analysis/ScopInfo.cpp:796
@@ +795,3 @@
+    assert(ICond &&
+           "Condition of exiting branch was neither constant nor ICmp!");
+
----------------
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!");
}

================
Comment at: lib/Analysis/ScopInfo.cpp:1497
@@ +1496,3 @@
+  while (L && !L->contains(EntryBB))
+    L = L->getParentLoop();
+  if (L)
----------------
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.

================
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;
+
----------------
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.

================
Comment at: lib/Analysis/ScopInfo.cpp:1506
@@ +1505,3 @@
+}
+
+void Scop::addBranchConstraintsToDomain(Region *R, unsigned PreScopLoopDepth,
----------------
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?





================
Comment at: lib/Analysis/ScopInfo.cpp:1523
@@ +1522,3 @@
+    isl_set *Domain = DomainMap[BB];
+    assert(Domain);
+
----------------
Maybe say that due to the use of RPostOrderTraversal the BB should already have a domain assigned, as all predecessors of it should already have been processed.

================
Comment at: lib/Analysis/ScopInfo.cpp:1543
@@ +1542,3 @@
+
+      Loop *SuccBBLoop = LI.getLoopFor(SuccBB);
+      if (BBLoop && SuccBBLoop != BBLoop &&
----------------
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?

================
Comment at: lib/Analysis/ScopInfo.cpp:1557
@@ +1556,3 @@
+        else
+          RemoveDim = true;
+
----------------
The two branches seem to be identical, why do you even have the second 'if'?


http://reviews.llvm.org/D12428