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

[gramanas]

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.

[gramanas]

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]

• Refactored the createInductionVariable() function to accept DebugLoc instead of Instruction.
• Get the debugLoc to set from the oldBasicBlock's induction variable if any.`

[gramanas]

• styling nit

[vsk]

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]

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]

• DebugLoc pass by value
• embed code in helper function

[vsk]

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.

[gramanas]

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.

[gramanas]

ping!

[xbolva00]

In D49347#1177914, @gramanas wrote:

ping!

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

[gramanas]

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

Yes, it passes without unexpected failures.

[vsk]

In D49347#1168565, @gramanas wrote:

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?

lib/Transforms/Vectorize/LoopVectorize.cpp

protected:
 
   /// Set up the values of the IVs correctly when exiting the vector loop.
   void fixupIVUsers(PHINode *OrigPhi, const InductionDescriptor &II,
                     Value *CountRoundDown, Value *EndValue,
                     BasicBlock *MiddleBlock);
 
   /// Create a new induction variable inside L.
   PHINode *createInductionVariable(Loop *L, Value *Start, Value *End,
-                                   Value *Step, Instruction *DL);
+                                   Value *Step, DebugLoc *OldInductionDL);
 
   /// Handle all cross-iteration phis in the header.
   void fixCrossIterationPHIs();
 
   /// Fix a first-order recurrence. This is the second phase of vectorizing
   /// this phi node.
   void fixFirstOrderRecurrence(PHINode *Phi);
 
if (auto *II = dyn_cast<IntrinsicInst>(Cloned))
     if (II->getIntrinsicID() == Intrinsic::assume)
       AC->registerAssumption(II);
 
   // End if-block.
   if (IfPredicateInstr)
     PredicatedInstructions.push_back(Cloned);
 }
 
-PHINode *InnerLoopVectorizer::createInductionVariable(Loop *L, Value *Start,
-                                                      Value *End, Value *Step,
-                                                      Instruction *DL) {
+PHINode *InnerLoopVectorizer::createInductionVariable(
+    Loop *L, Value *Start, Value *End, Value *Step, DebugLoc *OldInductionDL) {
   BasicBlock *Header = L->getHeader();
   BasicBlock *Latch = L->getLoopLatch();
   // As we're just creating this loop, it's possible no latch exists
   // yet. If so, use the header as this will be a single block loop.
   if (!Latch)
     Latch = Header;
 
   IRBuilder<> Builder(&*Header->getFirstInsertionPt());
-  Instruction *OldInst = getDebugLocFromInstOrOperands(OldInduction);
-  setDebugLocFromInst(Builder, OldInst);
+  Builder.SetCurrentDebugLocation(*OldInductionDL);
   auto *Induction = Builder.CreatePHI(Start->getType(), 2, "index");
 
   Builder.SetInsertPoint(Latch->getTerminator());
-  setDebugLocFromInst(Builder, OldInst);
+  Builder.SetCurrentDebugLocation(*OldInductionDL);
 
   // Create i+1 and fill the PHINode.
   Value *Next = Builder.CreateAdd(Induction, Step, "index.next");
   Induction->addIncoming(Start, L->getLoopPreheader());
   Induction->addIncoming(Next, Latch);
   // Create the compare.
   Value *ICmp = Builder.CreateICmpEQ(Next, End);
   Builder.CreateCondBr(ICmp, L->getExitBlock(), Header);
BasicBlock *InnerLoopVectorizer::createVectorizedLoopSkeleton() {
   // expressions.
   emitSCEVChecks(Lp, ScalarPH);
 
   // Generate the code that checks in runtime if arrays overlap. We put the
   // checks into a separate block to make the more common case of few elements
   // faster.
   emitMemRuntimeChecks(Lp, ScalarPH);
 
+  // OldInduction could be nullptr. In that case we want the Debug Location
+  // from the old induction phi to be passed on to the new one.
+  DebugLoc InductionDL;
+  if (!OldInduction) {
+    for (const auto &phi : OldBasicBlock->phis()) {
+      if (Legal->isInductionPhi(&phi)) {
+        InductionDL = phi.getDebugLoc();
+        break;
+      }
+    }
+  } else {
+    InductionDL = getDebugLocFromInstOrOperands(OldInduction)->getDebugLoc();
+  }
+
   // Generate the induction variable.
   // The loop step is equal to the vectorization factor (num of SIMD elements)
   // times the unroll factor (num of SIMD instructions).
   Value *CountRoundDown = getOrCreateVectorTripCount(Lp);
   Constant *Step = ConstantInt::get(IdxTy, VF * UF);
   Induction =
-      createInductionVariable(Lp, StartIdx, CountRoundDown, Step,
-                              getDebugLocFromInstOrOperands(OldInduction));
+      createInductionVariable(Lp, StartIdx, CountRoundDown, Step, &InductionDL);
 
   // We are going to resume the execution of the scalar loop.
   // Go over all of the induction variables that we found and fix the
   // PHIs that are left in the scalar version of the loop.
   // The starting values of PHI nodes depend on the counter of the last
   // iteration in the vectorized loop.
   // If we come from a bypass edge then we need to start from the original
   // start value.

test/Transforms/LoopVectorize/discriminator.ll

 }
 
 ;DBG_VALUE: ![[TOP:[0-9]*]] = distinct !DISubprogram(name: "foo"
 ;LOOPVEC_4_1: discriminator: 17
 ;LOOPVEC_2_3: discriminator: 25
 ;LOOPUNROLL_5: discriminator: 21
 ; When unrolling after loop vectorize, both vec_body and remainder loop
 ; are unrolled.
-;LOOPVEC_UNROLL: discriminator: 385
 ;LOOPVEC_UNROLL: discriminator: 9
+;LOOPVEC_UNROLL: discriminator: 385
 ;DBG_VALUE: ![[DBG]] = {{.*}}, scope: ![[TOP]]
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, debugInfoForProfiling: true)
 !1 = !DIFile(filename: "a.cc", directory: "/")
 !3 = !{i32 2, !"Dwarf Version", i32 4}

test/Transforms/LoopVectorize/i8-induction.ll

 ; RUN: opt < %s  -loop-vectorize -force-vector-interleave=1 -force-vector-width=4 -dce -instcombine -S
 ; RUN: opt < %s -debugify -loop-vectorize -S | FileCheck %s --check-prefix=DEBUGLOC
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 
 @a = common global i8 0, align 1
 @b = common global i8 0, align 1
 
 define void @f() nounwind uwtable ssp {
 ; Check that the induction phis and adds have debug location.
 ;
 ; DEBUGLOC-LABEL: vector.body:
-; DEBUGLOC:         %vec.ind = phi {{.*}}, !dbg ![[DbgLoc:[0-9]+]]
+; DEBUGLOC:         %index = phi {{.*}}, !dbg ![[DbgLoc:[0-9]+]]
+; DEBUGLOC:         %vec.ind = phi {{.*}}, !dbg ![[DbgLoc]]
+; DEBUGLOC:         %index.next = add {{.*}}, !dbg ![[DbgLoc]]
 ; DEBUGLOC:         %vec.ind.next = add {{.*}}, !dbg ![[DbgLoc]]
 
 scalar.ph:
   store i8 0, i8* inttoptr (i64 1 to i8*), align 1
   %0 = load i8, i8* @a, align 1
   br label %for.body
 
 for.body: