This is an archive of the discontinued LLVM Phabricator instance.

Traverse the SCoP to compute non-loop-carried domain conditions
ClosedPublic

Authored by jdoerfert on Aug 28 2015, 4:27 AM.

Details

Summary
In order to compute domain conditions for conditionals we will now
traverse the region in the ScopInfo once and build the domains for
each block in the region. The SCoP statements can then use these
constraints when they build their domain.

The reason behind this change is twofold:
  1) This removes a big chunk of preprocessing logic from the
     TempScopInfo, namely the Conditionals we used to build there.
     Additionally to moving this logic it is also simplified. Instead
     of walking the dominance tree up for each basic block in the
     region (as we did before), we now traverse the region only
     once in order to collect the domain conditions.
  2) This is the first step towards the isl based domain creation.
     The second step will traverse the region similar to this step,
     however it will propagate back edge conditions. Once both are in
     place this conditional handling will allow multiple exit loops
     additional logic.

Diff Detail

Repository
rL LLVM

Event Timeline

jdoerfert updated this revision to Diff 33411.Aug 28 2015, 4:27 AM
jdoerfert retitled this revision from to Traverse the SCoP to compute non-loop-carried domain conditions.
jdoerfert updated this object.
jdoerfert added a subscriber: Restricted Project.

Note: This passes lnt.

grosser edited edge metadata.Aug 28 2015, 5:59 AM

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

include/polly/ScopInfo.h
900 ↗(On Diff #33411)

@params

913 ↗(On Diff #33411)

@params?

920 ↗(On Diff #33411)

@params

1205 ↗(On Diff #33411)

@params

lib/Analysis/ScopInfo.cpp
779 ↗(On Diff #33411)

You can probably derive the Loop L from BI itself.

787 ↗(On Diff #33411)

What about ThenCondSet, ElseCondSet?

796 ↗(On Diff #33411)

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!");

}

1497 ↗(On Diff #33411)

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.

1502 ↗(On Diff #33411)

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.

1506 ↗(On Diff #33411)

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?

1523 ↗(On Diff #33411)

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.

1543 ↗(On Diff #33411)

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?

1557 ↗(On Diff #33411)

The two branches seem to be identical, why do you even have the second 'if'?

jdoerfert updated this revision to Diff 33543.Aug 30 2015, 12:58 PM
jdoerfert marked 7 inline comments as done.
jdoerfert edited edge metadata.

Some code beautifications and comments

Some comments for the new version.

include/polly/ScopInfo.h
900 ↗(On Diff #33411)

A different commit should introduce these.

lib/Analysis/ScopInfo.cpp
779 ↗(On Diff #33411)

Not without the loop info and I already got the loop anyway.

787 ↗(On Diff #33411)

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.

796 ↗(On Diff #33411)

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.

1497 ↗(On Diff #33411)

I use the getRelativeLoopDepth() now. That does the trick, hence this code is gone.

1502 ↗(On Diff #33411)

I think this makes the code simpler, so I'd suggest we change it at some later point. OK?

1506 ↗(On Diff #33411)

Maybe even a little ASCII art?

Michael spoiled you...

1543 ↗(On Diff #33411)

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.

grosser accepted this revision.Aug 30 2015, 1:09 PM
grosser edited edge metadata.

LGTM, only some minor typos.

include/polly/ScopInfo.h
914 ↗(On Diff #33543)

@brief

916 ↗(On Diff #33543)

'to argue'? maybe 'to question' or 'to obtain'?

918 ↗(On Diff #33543)

'void' at the end of the line?

lib/Analysis/ScopInfo.cpp
817 ↗(On Diff #33543)

Uppercase.

820 ↗(On Diff #33543)

Uppercase.

1515 ↗(On Diff #33543)

visit

1559 ↗(On Diff #33543)

careful

1574 ↗(On Diff #33543)

why the linebreak?

This revision is now accepted and ready to land.Aug 30 2015, 1:09 PM
This revision was automatically updated to reflect the committed changes.