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

[Johannes Doerfert]

On 09/15, Tobias Grosser wrote:
> 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.
I'll look into it and commit one way later today.

> Otherwise, LGTM.
> 
> Tobias

-- 

Johannes Doerfert
Researcher / PhD Student

Compiler Design Lab (Prof. Hack)
Saarland University, Computer Science
Building E1.3, Room 4.26

Tel. +49 (0)681 302-57521 : doerfert at cs.uni-saarland.de
Fax. +49 (0)681 302-3065  : http://www.cdl.uni-saarland.de/people/doerfert
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140915/6e70fdc2/attachment.sig>