[polly] r217508 - [Refactor] Cleanup isl code generation

[Tobias Grosser]

On 15/09/2014 11:32, Johannes Doerfert wrote:
> On 09/15, Tobias Grosser wrote:
>> >On 14/09/2014 18:45, Johannes Doerfert wrote:
>>> > >Attached a fix for the problem. Could you take a look?
>> >
>> >Thanks.
>> >
>>> > > From ee01feec4f9b7616f4a37fad7c71bd4f4d471d6e Mon Sep 17 00:00:00 2001
>>> > >From: Johannes Doerfert<doerfert at cs.uni-saarland.de>
>>> > >Date: Sun, 14 Sep 2014 18:41:57 +0200
>>> > >Subject: [PATCH] [Fix] Rewire the Region after a unconditional entry edge is
>>> > >  created
>>> > >
>>> > >   We use SplitEdge to split a conditional entry edge of the SCoP region.
>>> > >   However, SplitEdge can cause two different situations. This patch tests
>>> > >   which one is present and deals with the former unhandled one.
>> >
>> >It would be interesting to mention the actual issue in the commit message.
> I thought I did. The second, before not-handled way, SplitEdge can "split
> an edge".
>
>> >At a first look I thought we missed the update of the ScopStmt, but now
>> >looking again it seems as if we missed the simple case and incorrectly
>> >updated the ScoP. Is this right?
> No. Only the simple case was covered, thus the one were the SCoP doesn't
> need to be updated. Now both situations are implemented, the simple one
> (then branch) and the complicated one where we update the SCoP. I just
> moved this update mechanism into an own function as it is used twice now.

OK. Then I misunderstood. Now I really believe the patch makes sense. :-)

One more comment inline:

>> >Also, it would be good to have a test case that reproduces the problem.
> Yes, I will try to create one.
>
>> >Cheers,
>> >Tobias
>> >
>> >
>>> > >  lib/Support/ScopHelper.cpp | 52 ++++++++++++++++++++++++++++++++++++++--------
>>> > >  1 file changed, 43 insertions(+), 9 deletions(-)
>>> > >
>>> > >diff --git a/lib/Support/ScopHelper.cpp b/lib/Support/ScopHelper.cpp
>>> > >index 01c1dd0..0e2f6fa 100644
>>> > >--- a/lib/Support/ScopHelper.cpp
>>> > >+++ b/lib/Support/ScopHelper.cpp
>>> > >@@ -86,6 +86,17 @@ BasicBlock *polly::createSingleExitEdge(Region *R, Pass *P) {
>>> > >    return SplitBlockPredecessors(BB, Preds, ".region", P);
>>> > >  }
>>> > >
>>> > >+static void replaceScopAndRegionEntry(polly::Scop *S, BasicBlock *OldEntry,
>>> > >+                                      BasicBlock *NewEntry) {
>>> > >+  for (polly::ScopStmt *Stmt : *S)
>>> > >+    if (Stmt->getBasicBlock() == OldEntry) {
>>> > >+      Stmt->setBasicBlock(NewEntry);
>>> > >+      break;
>>> > >+    }
>>> > >+
>>> > >+  S->getRegion().replaceEntryRecursive(NewEntry);
>>> > >+}
>>> > >+
>>> > >  BasicBlock *polly::simplifyRegion(Scop *S, Pass *P) {
>>> > >    Region *R = &S->getRegion();
>>> > >
>>> > >@@ -96,20 +107,43 @@ BasicBlock *polly::simplifyRegion(Scop *S, Pass *P) {
>>> > >    if (!EnteringBB) {
>>> > >      BasicBlock *OldEntry = R->getEntry();
>>> > >      BasicBlock *NewEntry = SplitBlock(OldEntry, OldEntry->begin(), P);
>>> > >-
>>> > >-    for (ScopStmt *Stmt : *S)
>>> > >-      if (Stmt->getBasicBlock() == OldEntry) {
>>> > >-        Stmt->setBasicBlock(NewEntry);
>>> > >-        break;
>>> > >-      }
>>> > >-
>>> > >-    R->replaceEntryRecursive(NewEntry);
>>> > >+    replaceScopAndRegionEntry(S, OldEntry, NewEntry);

You update it here once.

>>> > >      EnteringBB = OldEntry;
>>> > >    }
>>> > >
>>> > >    // Create an unconditional entry edge.
>>> > >    if (EnteringBB->getTerminator()->getNumSuccessors() != 1) {
>>> > >-    EnteringBB = SplitEdge(EnteringBB, R->getEntry(), P);
>>> > >+    BasicBlock *EntryBB = R->getEntry();
>>> > >+    BasicBlock *SplitEdgeBB = SplitEdge(EnteringBB, EntryBB, P);
>>> > >+
>>> > >+    // Once the edge between EnteringBB and EntryBB is split, two cases arise.
>>> > >+    // The first is simple. The new block is inserted between EnteringBB and
>>> > >+    // EntryBB. In this case no further action is needed. However it might
>>> > >+    // happen that the new block is inserted __after__ EntryBB causing the
>>> > >+    // following situation:
>>> > >+    //
>>> > >+    // EnteringBB
>>> > >+    //     |
>>> > >+    //    / \
>>> > >+    //    |  \-> some_other_BB_not_in_R
>>> > >+    //    V
>>> > >+    // EntryBB
>>> > >+    //    |
>>> > >+    //    V
>>> > >+    // SplitEdgeBB
>>> > >+    //
>>> > >+    // In this case we need to swap the role of EntryBB and SplitEdgeBB.
>>> > >+
>>> > >+    // Check which case SplitEdge produced:
>>> > >+    if (SplitEdgeBB->getTerminator()->getSuccessor(0) == EntryBB) {
>>> > >+      // First (simple) case.
>>> > >+      EnteringBB = SplitEdgeBB;
>>> > >+    } else {
>>> > >+      // Second (complicated) case.
>>> > >+      replaceScopAndRegionEntry(S, EntryBB, SplitEdgeBB);

And here a second time.

>>> > >+      EnteringBB = EntryBB;
>>> > >+    }

Would it make sense to to just update it _here_ a single time to the 
final value?

If this is possible and actually simplifies the code, please change it.

Otherwise, LGTM.

Tobias