[GVN] Set proper debug locations for some instructions created by GVN.

[echristo]

The GVN ones seem reasonable since the instructions they're replacing are deleted, are the ones in PHITransAddr the same?

-eric

[samsonov]

In D10351#185934, @echristo wrote:

The GVN ones seem reasonable since the instructions they're replacing are deleted, are the ones in PHITransAddr the same?

You could say so. Instructions created in PHITransAddr would eventually be used here:

// Okay, we can eliminate this load by inserting a reload in the predecessor
// and using PHI construction to get the value in the other predecessors, do
// it.
DEBUG(dbgs() << "GVN REMOVING PRE LOAD: " << *LI << '\n');
DEBUG(if (!NewInsts.empty())
        dbgs() << "INSERTED " << NewInsts.size() << " INSTS: "
               << *NewInsts.back() << '\n');

[• dberlin]

No, they aren't, and it will in fact, often get the wrong location for this.

But given how GVN works, and the expectation that it has that
phitransaddr will insert instructions for it,
this is pretty much the best you can do (unless you want to do nothing).

This is because PHITransAddr does not have any clue in the world about
what computation it is really going to eventually replace, and GVN
can't tell it, because GVN doesn't know either.

(Basically, GVN is asking phi translate "here's a load, translate all
of it's parts". PHI Translate doesn't just do phi translation, it will
in fact, go off looking to see if something might have the same
operands as the result, etc).

It will be "mostly" right for "depth 1" phi translation, however.

[samsonov]

In D10351#185951, @dberlin wrote:

No, they aren't, and it will in fact, often get the wrong location for this.

But given how GVN works, and the expectation that it has that
phitransaddr will insert instructions for it,
this is pretty much the best you can do (unless you want to do nothing).

This is because PHITransAddr does not have any clue in the world about
what computation it is really going to eventually replace, and GVN
can't tell it, because GVN doesn't know either.

(Basically, GVN is asking phi translate "here's a load, translate all
of it's parts". PHI Translate doesn't just do phi translation, it will
in fact, go off looking to see if something might have the same
operands as the result, etc).

Right... But why is this wrong? Suppose GVN replaces a Load L in the basic block BB_3,
and the address computation for L involves values V1 from BB_1 (with location Loc_1) and V2 from BB_2 (with location Loc_2).

PHITransAddr basically just re-creates the address computation in some predecessor of BB_3.
Shouldn't instructions re-computing V1 and V2 be associated with the same locations Loc_1 and Loc_2 as the original values?

It will be "mostly" right for "depth 1" phi translation, however.

llvm/trunk/lib/Analysis/PHITransAddr.cpp

InsertPHITranslatedSubExpr(Value *InVal, BasicBlock *CurBB,
   // Handle cast of PHI translatable value.
   if (CastInst *Cast = dyn_cast<CastInst>(Inst)) {
     if (!isSafeToSpeculativelyExecute(Cast)) return nullptr;
     Value *OpVal = InsertPHITranslatedSubExpr(Cast->getOperand(0),
                                               CurBB, PredBB, DT, NewInsts);
     if (!OpVal) return nullptr;
 
     // Otherwise insert a cast at the end of PredBB.
-    CastInst *New = CastInst::Create(Cast->getOpcode(),
-                                     OpVal, InVal->getType(),
-                                     InVal->getName()+".phi.trans.insert",
+    CastInst *New = CastInst::Create(Cast->getOpcode(), OpVal, InVal->getType(),
+                                     InVal->getName() + ".phi.trans.insert",
                                      PredBB->getTerminator());
+    New->setDebugLoc(Inst->getDebugLoc());
     NewInsts.push_back(New);
     return New;
   }
 
   // Handle getelementptr with at least one PHI operand.
   if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(Inst)) {
     SmallVector<Value*, 8> GEPOps;
     BasicBlock *CurBB = GEP->getParent();
     for (unsigned i = 0, e = GEP->getNumOperands(); i != e; ++i) {
       Value *OpVal = InsertPHITranslatedSubExpr(GEP->getOperand(i),
                                                 CurBB, PredBB, DT, NewInsts);
       if (!OpVal) return nullptr;
       GEPOps.push_back(OpVal);
     }
 
     GetElementPtrInst *Result = GetElementPtrInst::Create(
         GEP->getSourceElementType(), GEPOps[0], makeArrayRef(GEPOps).slice(1),
         InVal->getName() + ".phi.trans.insert", PredBB->getTerminator());
+    Result->setDebugLoc(Inst->getDebugLoc());
     Result->setIsInBounds(GEP->isInBounds());
     NewInsts.push_back(Result);
     return Result;
   }
 
 #if 0
   // FIXME: This code works, but it is unclear that we actually want to insert
   // a big chain of computation in order to make a value available in a block.

llvm/trunk/lib/Transforms/Scalar/GVN.cpp

for (const auto &PredLoad : PredLoads) {
     DEBUG(dbgs() << "GVN INSERTED " << *NewLoad << '\n');
   }
 
   // Perform PHI construction.
   Value *V = ConstructSSAForLoadSet(LI, ValuesPerBlock, *this);
   LI->replaceAllUsesWith(V);
   if (isa<PHINode>(V))
     V->takeName(LI);
+  if (Instruction *I = dyn_cast<Instruction>(V))
+    I->setDebugLoc(LI->getDebugLoc());
   if (V->getType()->getScalarType()->isPointerTy())
     MD->invalidateCachedPointerInfo(V);
   markInstructionForDeletion(LI);
   ++NumPRELoad;
   return true;
 }
 
 /// Attempt to eliminate a load whose dependencies are
if (UnavailableBlocks.empty()) {
     DEBUG(dbgs() << "GVN REMOVING NONLOCAL LOAD: " << *LI << '\n');
 
     // Perform PHI construction.
     Value *V = ConstructSSAForLoadSet(LI, ValuesPerBlock, *this);
     LI->replaceAllUsesWith(V);
 
     if (isa<PHINode>(V))
       V->takeName(LI);
+    if (Instruction *I = dyn_cast<Instruction>(V))
+      I->setDebugLoc(LI->getDebugLoc());
     if (V->getType()->getScalarType()->isPointerTy())
       MD->invalidateCachedPointerInfo(V);
     markInstructionForDeletion(LI);
     ++NumGVNLoad;
     return true;
   }
 
   // Step 4: Eliminate partial redundancy.

llvm/trunk/test/Transforms/GVN/phi-translate.ll

 ; RUN: opt -basicaa -gvn -S < %s | FileCheck %s
 
 target datalayout = "e-p:64:64:64"
 
 ; CHECK-LABEL: @foo(
 ; CHECK: entry.end_crit_edge:
-; CHECK:   %n.pre = load i32, i32* %q.phi.trans.insert
+; CHECK:   %j.phi.trans.insert = sext i32 %x to i64, !dbg [[J_LOC:![0-9]+]]
+; CHECK:   %q.phi.trans.insert = getelementptr {{.*}}, !dbg [[Q_LOC:![0-9]+]]
+; CHECK:   %n.pre = load i32, i32* %q.phi.trans.insert, !dbg [[N_LOC:![0-9]+]]
 ; CHECK: then:
 ; CHECK:   store i32 %z
 ; CHECK: end:
-; CHECK:   %n = phi i32 [ %n.pre, %entry.end_crit_edge ], [ %z, %then ]
+; CHECK:   %n = phi i32 [ %n.pre, %entry.end_crit_edge ], [ %z, %then ], !dbg [[N_LOC]]
 ; CHECK:   ret i32 %n
 
+; CHECK-DAG: [[J_LOC]] = !DILocation(line: 45, column: 1, scope: !{{.*}})
+; CHECK-DAG: [[Q_LOC]] = !DILocation(line: 46, column: 1, scope: !{{.*}})
+; CHECK-DAG: [[N_LOC]] = !DILocation(line: 47, column: 1, scope: !{{.*}})
+
 @G = external global [100 x i32]
 define i32 @foo(i32 %x, i32 %z) {
 entry:
-  %tobool = icmp eq i32 %z, 0
-  br i1 %tobool, label %end, label %then
+  %tobool = icmp eq i32 %z, 0, !dbg !7
+  br i1 %tobool, label %end, label %then, !dbg !7
 
 then:
-  %i = sext i32 %x to i64
-  %p = getelementptr [100 x i32], [100 x i32]* @G, i64 0, i64 %i
-  store i32 %z, i32* %p
-  br label %end
+  %i = sext i32 %x to i64, !dbg !8
+  %p = getelementptr [100 x i32], [100 x i32]* @G, i64 0, i64 %i, !dbg !8
+  store i32 %z, i32* %p, !dbg !8
+  br label %end, !dbg !8
 
 end:
-  %j = sext i32 %x to i64
-  %q = getelementptr [100 x i32], [100 x i32]* @G, i64 0, i64 %j
-  %n = load i32, i32* %q
-  ret i32 %n
+  %j = sext i32 %x to i64, !dbg !9
+  %q = getelementptr [100 x i32], [100 x i32]* @G, i64 0, i64 %j, !dbg !10
+  %n = load i32, i32* %q, !dbg !11
+  ret i32 %n, !dbg !11
 }
+
+!llvm.module.flags = !{!0, !1, !2}
+!0 = !{i32 2, !"Dwarf Version", i32 4}
+!1 = !{i32 2, !"Debug Info Version", i32 3}
+!2 = !{i32 1, !"PIC Level", i32 2}
+
+!3 = !{}
+!4 = !DISubroutineType(types: !3)
+!5 = !DIFile(filename: "a.cc", directory: "/tmp")
+!6 = !DISubprogram(name: "foo", scope: !5, file: !5, line: 42, type: !4, isLocal: false, isDefinition: true, scopeLine: 43, flags: DIFlagPrototyped, isOptimized: false, function: i32 (i32, i32)* @foo, variables: !3)
+!7 = !DILocation(line: 43, column: 1, scope: !6)
+!8 = !DILocation(line: 44, column: 1, scope: !6)
+!9 = !DILocation(line: 45, column: 1, scope: !6)
+!10 = !DILocation(line: 46, column: 1, scope: !6)
+!11 = !DILocation(line: 47, column: 1, scope: !6)