[globalisel][irtranslator] Verify that DILocations aren't lost in translation

[dsanders]

Summary

Also fix a couple bugs where DILocations are lost. EntryBuilder wasn't passing
on debug locations for PHI's, constants, GLOBAL_VALUE, etc.

[vsk]

The verifier bits look great, I'll let someone else comment on the functionality changes. Thanks!

test/CodeGen/AArch64/GlobalISel/irtranslator-dilocation.ll

33 ↗

(On Diff #171219)

It's probably safe to prune all of these out.

[aemerson]

lib/CodeGen/GlobalISel/IRTranslator.cpp

124 ↗

(On Diff #171219)

I think this debug print is probably unnecessary given it'll print for every instruction translated (and without ASSERTIONS it won't actually do anything).

1742 ↗

(On Diff #171219)

This is unnecessary as Verifier goes out of scope immediately after?

[dsanders]

lib/CodeGen/GlobalISel/IRTranslator.cpp

124 ↗

(On Diff #171219)

I'm not sure I understand this comment. It sounds like you're suggesting its removal but the follow on implies the right thing to do is to wrap it in '#ifndef NDEBUG'.

1742 ↗

(On Diff #171219)

Good point

[aemerson]

lib/CodeGen/GlobalISel/IRTranslator.cpp

124 ↗

(On Diff #171219)

Yes I was suggesting removing it.

AFAIK NDEBUG and assertions are independent of each other, so without assertions enabled this will print a message that doesn't actually check anything. Could be wrong about that.

[dsanders]

lib/CodeGen/GlobalISel/IRTranslator.cpp

124 ↗

(On Diff #171219)

I'd prefer not to remove it as it was quite useful to see what DILocations were expected to go where when I was debugging the missing DILocations that are fixed in this patch. I do agree it should only print "Checking ..." when the checks are actually happening though.

AFAIK NDEBUG and assertions are independent of each other, so without assertions enabled this will
print a message that doesn't actually check anything. Could be wrong about that.

The effects of LLVM_ENABLE_ASSERTIONS are achieved by adding -UNDEBUG and removing any -DNDEBUG from the standard build flags. The code for that is in HandleLLVMOptions.cmake

[aemerson]

LGTM with the nits addressed.

llvm/trunk/lib/CodeGen/GlobalISel/IRTranslator.cpp

static void reportTranslationError(MachineFunction &MF,
   else
     ORE.emit(R);
 }
 
 IRTranslator::IRTranslator() : MachineFunctionPass(ID) {
   initializeIRTranslatorPass(*PassRegistry::getPassRegistry());
 }
 
+#ifndef NDEBUG
+/// Verify that every instruction created has the same DILocation as the
+/// instruction being translated.
+class DILocationVerifier : MachineFunction::Delegate {
+  MachineFunction &MF;
+  const Instruction *CurrInst = nullptr;
+
+public:
+  DILocationVerifier(MachineFunction &MF) : MF(MF) { MF.setDelegate(this); }
+  ~DILocationVerifier() { MF.resetDelegate(this); }
+
+  const Instruction *getCurrentInst() const { return CurrInst; }
+  void setCurrentInst(const Instruction *Inst) { CurrInst = Inst; }
+
+  void MF_HandleInsertion(const MachineInstr &MI) override {
+    assert(getCurrentInst() && "Inserted instruction without a current MI");
+
+    // Only print the check message if we're actually checking it.
+#ifndef NDEBUG
+    LLVM_DEBUG(dbgs() << "Checking DILocation from " << *CurrInst
+                      << " was copied to " << MI);
+#endif
+    assert(CurrInst->getDebugLoc() == MI.getDebugLoc() &&
+           "Line info was not transferred to all instructions");
+  }
+  void MF_HandleRemoval(const MachineInstr &MI) override {}
+};
+#endif // ifndef NDEBUG
+
+
 void IRTranslator::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.addRequired<StackProtector>();
   AU.addRequired<TargetPassConfig>();
   getSelectionDAGFallbackAnalysisUsage(AU);
   MachineFunctionPass::getAnalysisUsage(AU);
 }
 
 static void computeValueLLTs(const DataLayout &DL, Type &Ty,
MIRBuilder.buildAtomicRMW(
       *MF->getMachineMemOperand(MachinePointerInfo(I.getPointerOperand()),
                                 Flags, DL->getTypeStoreSize(ResType),
                                 getMemOpAlignment(I), AAMDNodes(), nullptr,
                                 I.getSyncScopeID(), I.getOrdering()));
   return true;
 }
 
 void IRTranslator::finishPendingPhis() {
+#ifndef NDEBUG
+  DILocationVerifier Verifier(*MF);
+#endif // ifndef NDEBUG
   for (auto &Phi : PendingPHIs) {
     const PHINode *PI = Phi.first;
     ArrayRef<MachineInstr *> ComponentPHIs = Phi.second;
+    EntryBuilder.setDebugLoc(PI->getDebugLoc());
+#ifndef NDEBUG
+    Verifier.setCurrentInst(PI);
+#endif // ifndef NDEBUG
 
     // All MachineBasicBlocks exist, add them to the PHI. We assume IRTranslator
     // won't create extra control flow here, otherwise we need to find the
     // dominating predecessor here (or perhaps force the weirder IRTranslators
     // to provide a simple boundary).
     SmallSet<const BasicBlock *, 4> HandledPreds;
 
     for (unsigned i = 0; i < PI->getNumIncomingValues(); ++i) {
bool IRTranslator::valueIsSplit(const Value &V,
   if (Offsets && !Offsets->empty())
     Offsets->clear();
   computeValueLLTs(*DL, *V.getType(), SplitTys, Offsets);
   return SplitTys.size() > 1;
 }
 
 bool IRTranslator::translate(const Instruction &Inst) {
   CurBuilder.setDebugLoc(Inst.getDebugLoc());
+  EntryBuilder.setDebugLoc(Inst.getDebugLoc());
   switch(Inst.getOpcode()) {
 #define HANDLE_INST(NUM, OPCODE, CLASS) \
     case Instruction::OPCODE: return translate##OPCODE(Inst, CurBuilder);
 #include "llvm/IR/Instruction.def"
   default:
     return false;
   }
 }
if (!valueIsSplit(*ArgIt, VMap.getOffsets(*ArgIt))) {
       VRegs.push_back(VArg);
     } else {
       unpackRegs(*ArgIt, VArg, EntryBuilder);
     }
     ArgIt++;
   }
 
   // Need to visit defs before uses when translating instructions.
+  {
     ReversePostOrderTraversal<const Function *> RPOT(&F);
+#ifndef NDEBUG
+    DILocationVerifier Verifier(*MF);
+#endif // ifndef NDEBUG
     for (const BasicBlock *BB : RPOT) {
       MachineBasicBlock &MBB = getMBB(*BB);
       // Set the insertion point of all the following translations to
       // the end of this basic block.
       CurBuilder.setMBB(MBB);
 
       for (const Instruction &Inst : *BB) {
+#ifndef NDEBUG
+        Verifier.setCurrentInst(&Inst);
+#endif // ifndef NDEBUG
         if (translate(Inst))
           continue;
 
         OptimizationRemarkMissed R("gisel-irtranslator", "GISelFailure",
                                    Inst.getDebugLoc(), BB);
         R << "unable to translate instruction: " << ore::NV("Opcode", &Inst);
 
         if (ORE->allowExtraAnalysis("gisel-irtranslator")) {
           std::string InstStrStorage;
           raw_string_ostream InstStr(InstStrStorage);
           InstStr << Inst;
 
           R << ": '" << InstStr.str() << "'";
         }
 
         reportTranslationError(*MF, *TPC, *ORE, R);
         return false;
       }
     }
+  }
 
   finishPendingPhis();
 
   // Merge the argument lowering and constants block with its single
   // successor, the LLVM-IR entry block.  We want the basic block to
   // be maximal.
   assert(EntryBB->succ_size() == 1 &&
          "Custom BB used for lowering should have only one successor");

llvm/trunk/test/CodeGen/AArch64/GlobalISel/irtranslator-dilocation.ll

+; RUN: llc -O0 -mtriple=aarch64-apple-ios -global-isel -debug-only=irtranslator \
+; RUN:     -stop-after=irtranslator %s -o - 2>&1 | FileCheck %s
+
+; CHECK: Checking DILocation from   %retval = alloca i32, align 4 was copied to G_FRAME_INDEX
+; CHECK: Checking DILocation from   %rv = alloca i32, align 4 was copied to G_FRAME_INDEX
+; CHECK: Checking DILocation from   store i32 0, i32* %retval, align 4 was copied to G_CONSTANT
+; CHECK: Checking DILocation from   store i32 0, i32* %retval, align 4 was copied to G_STORE
+; CHECK: Checking DILocation from   store i32 0, i32* %rv, align 4, !dbg !12 was copied to G_STORE debug-location !12; t.cpp:2:5
+; CHECK: Checking DILocation from   %0 = load i32, i32* %rv, align 4, !dbg !13 was copied to G_LOAD debug-location !13; t.cpp:3:8
+; CHECK: Checking DILocation from   ret i32 %0, !dbg !14 was copied to COPY debug-location !14; t.cpp:3:1
+; CHECK: Checking DILocation from   ret i32 %0, !dbg !14 was copied to RET_ReallyLR implicit $w0, debug-location !14; t.cpp:3:1
+
+source_filename = "t.cpp"
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "arm64-unknown-linux-gnu"
+
+; Function Attrs: noinline norecurse nounwind optnone
+define dso_local i32 @main() !dbg !7 {
+entry:
+  %retval = alloca i32, align 4
+  %rv = alloca i32, align 4
+  store i32 0, i32* %retval, align 4
+  call void @llvm.dbg.declare(metadata i32* %rv, metadata !11, metadata !DIExpression()), !dbg !12
+  store i32 0, i32* %rv, align 4, !dbg !12
+  %0 = load i32, i32* %rv, align 4, !dbg !13
+  ret i32 %0, !dbg !14
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 8.0.0 (trunk) (llvm/trunk 344296)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "t.cpp", directory: "/Volumes/Data/llvm.org/svn/build")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 8.0.0 (trunk) (llvm/trunk 344296)"}
+!7 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 1, type: !8, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: false, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DILocalVariable(name: "rv", scope: !7, file: !1, line: 2, type: !10)
+!12 = !DILocation(line: 2, column: 5, scope: !7)
+!13 = !DILocation(line: 3, column: 8, scope: !7)
+!14 = !DILocation(line: 3, column: 1, scope: !7)
+