Refactor loop peeling code for calculating phi invariance. Refactor loop peeling code by moving code for calculating phi invariance into a separate class that does the calculation. Redescribe and rework the algorithm in preparation for adding increased functionality. Add test case that does not exhibit peeling that will be subsequently supported.
Details
- Reviewers
mkazantsev - Group Reviewers
Restricted Project - Commits
- rGbe1ff1fe58b0: [NFC] Refactor loop peeling code for calculating phi invariance.
Diff Detail
Event Timeline
General notion: if your claim is that the existing code isn't handling some case, then you need to demonstrate it in a test. Steps are the following:
- Create a test, run utils/update_test_checks.py on it with existing implementation, and commit it;
- With your change, run this script again and re-generate checks so that they clearly show the difference;
Without it, it's hard to say how much better it became.
Also think if it can be split into several patches, some being NFC (e.g. factor out this logic into a separate class), which I'm generally fine with, and semantically meaningful functionality changes.
llvm/test/Transforms/LoopUnroll/peel-loop-phi-analysis.ll | ||
---|---|---|
7 | This is in the process of deprecation, pls only use new PM. | |
17 | I would strongly prefer auto-generated full checks over anything else. Please use utils/update_test_checks.py. |
I have split out the refactoring of the code so there are no intended
functional changes. I have added the test case as requested, showing
the current, limited functionality. After this lands, I will post a
patch with the expanded functionality.
I'm generally fine with this refactoring. Some style comments, the most important one being regarding using a constant to represent infinity instead of None.
For test -- LGTM, you can commit it separately, it doesn't require this NFC to go with it.
llvm/lib/Transforms/Utils/LoopPeel.cpp | ||
---|---|---|
107 | "Deterministic" isn't the best word here, because Phi value is always determined by its incoming block. How about "loop-invariant"? | |
118 | If it wasn't your intention, please change condition i > 10 here, otherwise we can say that it's (maybe) also profitable to peel off 10 iterations to get rid of this check (and it might be even more important than what are you describing). | |
167 | Can it be const Loop * here and in other places? Looks like this thing isn't intended to modify the code. | |
169 | "allowable maximum" -> "sufficient minimum"? We could off course peel more, just don't see a reason to. | |
175 | Do we really need that, provided that there is L->getLatch()? | |
179 | Can it return Infinity + 1 if MaxIterations > Infinity? Or, if it's impossible, please assert it here explicitly. | |
187 | Doesn't really look like it's worth storing it separately, it's cheaply claimed from loop when needed. | |
191 | Does this mean that this Phi provably never becomes invariant, or it can also mean that we haven't found this number (as it was in the original code)? I think the original code was structured better here for 2 reasons:
I'd suggest returning Optional<unsigned> as a safer structure for this. | |
197 | Please use DenseMap unless there are strong reasons to choose a std:: container. | |
200 | I don't think you ever expect nullptr loops to be a normal input. Change to const Loop &? | |
210 | LP -> L, M -> MaxIterations? |
@mkazantsev, I have addressed your concerns.
Regarding your comments concerning the use of infinity (as an unsigned) vs Optional<unsigned>: perhaps it's just me, but I found the use of Optional<unsigned> confusing and it became more confusing when I incorporated the max iterations into the algorithm. It isn't clear from the code at this point, but when one expands the algorithm to include binary operators, one wants to avoid computing the number of peels unnecessarily when the first operand has already reached (or surpassed) the maximum allowable peels. This is why I simplified the code to use an unsigned and placed it all within protected members of the class, wrapping it with the Optional<unsigned> on coming out, to both clarify the algorithm (especially when considering max iterations) and also to prevent people monkeying with the unsigned value (as you pointed out as a possibility in your comments). Your criticism of the constant name Infinity is valid--it is misleading. In response, I have hidden the Optional<unsigned> behind a type called PeelCount, used a constant Unknown (which is None) rather than Infinity and kept the math in protected functions. I think this makes the code easier to understand and also addresses your preference for using Optional<unsigned>.
llvm/lib/Transforms/Utils/LoopPeel.cpp | ||
---|---|---|
118 | I have removed this as it isn't germane to the example. I think I originally had it in to avoid partial loop unrolling, which I think is attempted before peeling |
"Deterministic" isn't the best word here, because Phi value is always determined by its incoming block. How about "loop-invariant"?