This is an archive of the discontinued LLVM Phabricator instance.

[WIP][DebugInfo][LV] DebugLoc in induction PHI
Needs ReviewPublic

Authored by gramanas on Jul 15 2018, 6:06 AM.

Details

Reviewers
vsk
mkuper
hsaito
Summary

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.

Diff Detail

Event Timeline

gramanas created this revision.Jul 15 2018, 6:06 AM
gramanas added inline comments.Jul 15 2018, 6:12 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
2571

L->getStartLoc() doesn't seem to be the correct debug loc for the phi and add instructions below. I've been using the same test as rL336667 for the tests and I think the correct DL to attach to these instructions is the one from %mul16 = phi i8.

@vsk what do you think?

2572

This is redundant as the variable DL has the exact same pointer as OldInst, that gets it from the call to createInductionVariable

gramanas updated this revision to Diff 155704.Jul 16 2018, 9:38 AM
  • Refactored the createInductionVariable() function to accept DebugLoc instead of Instruction.
  • Get the debugLoc to set from the oldBasicBlock's induction variable if any.`
gramanas updated this revision to Diff 155705.Jul 16 2018, 9:40 AM
  • styling nit
vsk added a comment.Jul 16 2018, 12:32 PM

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
470

DebugLoc can be passed by value cheaply. You don't need a pointer here.

2571

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.

2887

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?

2898

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.

2899

Please follow the style and structure of the surrounding code. Extract the logic to find the right location into a static helper.

gramanas retitled this revision from [WIP][DebugInfo][InstCombine] DebugLoc in induction PHI to [WIP][DebugInfo][LV] DebugLoc in induction PHI.Jul 17 2018, 6:51 AM
gramanas added inline comments.Jul 17 2018, 7:28 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
2887

The LoopVectorizationLegality class that determines what the PrimaryInduction variable will be. As I understand it, there are three cases:

  • There exists a primaryInduction
  • there might not be a PrimaryInduction at all
  • The widest instruction in the loop doesn't have the same type as the PrimaryInduction.

In the last two cases the PrimaryInduction will be nullptr.

The OldInduction in the createVectroziedLoopSkeleton() is this PrimaryInduction.

is it guaranteed to be in the original loop header?

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.

2898

We should take this opportunity to stop using this API

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?

gramanas updated this revision to Diff 155898.Jul 17 2018, 9:11 AM
  • DebugLoc pass by value
  • embed code in helper function
vsk added a comment.Jul 17 2018, 10:03 AM

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
2898

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.

ping!

Did you try "ninja check-all"? Everything ok?

Did you try "ninja check-all"? Everything ok?

Yes, it passes without unexpected failures.

vsk added a comment.EditedJul 31 2018, 9:40 AM

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.

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?