[polly] r217508 - [Refactor] Cleanup isl code generation
Johannes Doerfert
doerfert at cs.uni-saarland.de
Mon Sep 15 11:44:25 PDT 2014
Done in 217802. Thanks for noticing!
On 09/15, Johannes Doerfert wrote:
> 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
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
--
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/0bddd130/attachment.sig>
More information about the llvm-commits
mailing list