Instruction *DL can sometimes be nullptr at the time of the createInductionVariable() call.
Attach the start of the loop as DebugLoc in the induction phi and add if that's the case.
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 20450 Build 20450: arc lint + arc unit
Event Timeline
- Refactored the createInductionVariable() function to accept DebugLoc instead of Instruction.
- Get the debugLoc to set from the oldBasicBlock's induction variable if any.`
Thanks! Please add a few reviewers who are actively working on LV.
Consider retitling this to include '[LV]' instead of '[InstCombine]'? More comments inline --
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
475 | DebugLoc can be passed by value cheaply. You don't need a pointer here. | |
2597 | Hm, it makes sense that the starting location of the loop doesn't match the location of one of its IVs. As for which location is correct, see my other comments. | |
2913 | Please specify why OldInduction could be nullptr: there is no primary induction variable. In that case, what is the meaning of "the old induction phi"? Is there exactly one? If so, is it guaranteed to be in the original loop header? | |
2924 | The idea behind 'getDebugLocFromInstOrOperands' seems unsound to me. An instruction and its operands aren't interchangeable in general, which is why it's not a good idea to pretend that their debug locations are interchangeable. We should take this opportunity to stop using this API. | |
2925 | Please follow the style and structure of the surrounding code. Extract the logic to find the right location into a static helper. |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
2913 | The LoopVectorizationLegality class that determines what the PrimaryInduction variable will be. As I understand it, there are three cases:
In the last two cases the PrimaryInduction will be nullptr. The OldInduction in the createVectroziedLoopSkeleton() is this PrimaryInduction.
Since the function uses BasicBlock *OldBasicBlock = OrigLoop->getHeader(); BasicBlock *VectorPH = OrigLoop->getLoopPreheader(); BasicBlock *ExitBlock = OrigLoop->getExitBlock(); I am not sure it could be anywhere else other than the OldBasicBlock. From what I gather if there is a PrimaryInduction the new Induction variable should have it's DL, I am not sure what should happen in the case there is no PrimaryInduction. | |
2924 |
I agree, but simply using OldInduction->getDebugLoc() causes the test/Transforms/LoopVectorize/debugloc.ll to fail. After looking at it a bit more, it seems that this test is not particularly useful. The induction phi doesn't have debug location so the call to this function is justified by taking the DL from the br instruction. Sadly there is no source available to recreate the test without the DI embedded. Do you think I should make a new one? |
Can there be multiple induction phis in the original loop's header? If so, why is picking the location from the first phi correct?
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
2924 | Is the test checking for a behavior we actually want? In this case I'd say probably not. The behavior is strange: we pick the location of a branch instruction by accident, and it's not clear that it would map back to the original source well. I think it'd be appropriate to update or replace this test. |
Can there be multiple induction phis in the original loop's header? If so, why is picking the location from the first phi correct?
I am not sure. I don't think there can be many induction phis in a loop that hasn't been vectorized yet.
While creating the vectorized loop skeleton, createInductuonVariable() is called once, so I thought the first phi
found will probably be the only one as well.
Did you try "ninja check-all"? Everything ok?
Yes, it passes without unexpected failures.
Why not? The impression I get from skimming IndVarSimplify is that any phi in the loop header may be an induction variable.
While creating the vectorized loop skeleton, createInductuonVariable() is called once, so I thought the first phi
found will probably be the only one as well.
It doesn't hurt to try and verify this. Why not add an assertion that there are no more phis -- since if there are the DebugLoc selection would be suspect -- and test it in a stage2 build?
DebugLoc can be passed by value cheaply. You don't need a pointer here.