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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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 |
1557 ↗ | (On Diff #33411) | The two branches seem to be identical, why do you even have the second 'if'? |
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) |
Michael spoiled you... |
1543 ↗ | (On Diff #33411) |
Yes.
I am not sure which loop entry you refere to but I don't think that you can.
I don't think that you can get rid of it. |
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? |