[DebugInfo] Make sure CodeGenPrepare does not drop MD references to locals.

[wolfgangp]

Summary

This addresses PR39845.

CGP (CodeGenPrepare) performs a variety of optimizations using a transactional approach, i.e. it makes changes to the instructions and rolls these changes back when it finds that they don't improve the code. In particular, the undoing involves the reversal or RAUWs, which involves the tracking of uses of a value along with replacing these uses with the original value.

Currently Metadata uses are not tracked, however. In particular, any references from dbg.value instructions to locals that were RAUWed by the initial optimization attempt are not restored, negatively affecting debug info.

This patch tries to at least partially solve this problem by performing a reverse RAUW to undo the substitution, but this is obviously only correct when the replacement value was not referenced by Metadata at the time of the first replacement.
The complete solution would require keeping track of existing MD-uses of the replacement value, something I didn't want to attempt without consultation of someone with more in-depth knowledge of Metadata handling. However, the patch should improve a lot of cases, as many values created by CGP's optimizations are newly created.

Any suggestions on how to handle this better are welcome.

[dexonsmith]

Resigning as reviewer since I'm not sure I'll have much to offer here, but @aprantl feel free to pull me back in.

[aprantl]

lib/CodeGen/CodeGenPrepare.cpp

2365 ↗

(On Diff #177060)

I find is used by Metadata confusing, because then I'm thinking of an actual numbered MDNode. Perhaps say by a DbgInfoIntrinsic instead, since this is what we are really interested in?

2382 ↗

(On Diff #177060)

Can you also add a sentence about why we would want to undo those replacements?

[vsk]

Thanks for the patch, this is a great catch.

lib/CodeGen/CodeGenPrepare.cpp

2365 ↗

(On Diff #177060)

Seems like "by some ValueAsMetadata" would be more precise, as this isn't limited to debug info.

2390 ↗

(On Diff #177060)

Is it really uncommon for both Inst and New to be used by metadata? If not, it might be nice to tackle this fixme. Some modified version of findDbgUsers should help do the job.

[wolfgangp]

lib/CodeGen/CodeGenPrepare.cpp

2382 ↗

(On Diff #177060)

The "why" is directly mandated by the transaction-style design of the optimizations in CGP. Perhaps I'll rephrase the second sentence.

2390 ↗

(On Diff #177060)

Let me try and collect some statistics on this. In any case, my concern was not so much finding the nodes as doing the replacement, as this seems a bit involved, but looking at findDbgUses() helps. Thanks.

[wolfgangp]

Revised to explicitly keep track of uses in dbg value intrinsics . This is more straightforward and does not miss any edge cases as in the previous implementation. Thanks to Vedant for the suggestion.

[aprantl]

LGTM with outstanding comment addressed.

lib/CodeGen/CodeGenPrepare.cpp

2396 ↗

(On Diff #177622)

Could you perhaps also add why reinstating is the right thing to do into the comment? Otherwise this will look confusing to future readers.

llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp

struct InstructionAndIdx {
       unsigned Idx;
 
       InstructionAndIdx(Instruction *Inst, unsigned Idx)
           : Inst(Inst), Idx(Idx) {}
     };
 
     /// Keep track of the original uses (pair Instruction, Index).
     SmallVector<InstructionAndIdx, 4> OriginalUses;
+    /// Keep track of the debug users.
+    SmallVector<DbgValueInst *, 1> DbgValues;
 
     using use_iterator = SmallVectorImpl<InstructionAndIdx>::iterator;
 
   public:
     /// Replace all the use of \p Inst by \p New.
     UsesReplacer(Instruction *Inst, Value *New) : TypePromotionAction(Inst) {
       LLVM_DEBUG(dbgs() << "Do: UsersReplacer: " << *Inst << " with " << *New
                         << "\n");
       // Record the original uses.
       for (Use &U : Inst->uses()) {
         Instruction *UserI = cast<Instruction>(U.getUser());
         OriginalUses.push_back(InstructionAndIdx(UserI, U.getOperandNo()));
       }
+      // Record the debug uses separately. They are not in the instruction's
+      // use list, but they are replaced by RAUW.
+      findDbgValues(DbgValues, Inst);
+
       // Now, we can replace the uses.
       Inst->replaceAllUsesWith(New);
     }
 
     /// Reassign the original uses of Inst to Inst.
     void undo() override {
       LLVM_DEBUG(dbgs() << "Undo: UsersReplacer: " << *Inst << "\n");
       for (use_iterator UseIt = OriginalUses.begin(),
                         EndIt = OriginalUses.end();
            UseIt != EndIt; ++UseIt) {
         UseIt->Inst->setOperand(UseIt->Idx, Inst);
       }
+      // RAUW has replaced all original uses with references to the new value,
+      // including the debug uses. Since we are undoing the replacements,
+      // the original debug uses must also be reinstated to maintain the
+      // correctness and utility of debug value instructions.
+      for (auto *DVI: DbgValues) {
+        LLVMContext &Ctx = Inst->getType()->getContext();
+        auto *MV = MetadataAsValue::get(Ctx, ValueAsMetadata::get(Inst));
+        DVI->setOperand(0, MV);
+      }
     }
   };
 
   /// Remove an instruction from the IR.
   class InstructionRemover : public TypePromotionAction {
     /// Original position of the instruction.
     InsertionHandler Inserter;
 

llvm/trunk/test/DebugInfo/Generic/codegenprep-value.ll

+; RUN: opt -codegenprepare -S %s | FileCheck %s
+;
+; Generated from the following source with:
+; clang -O2 -g -S -emit-llvm -mllvm -stop-after=indirectbr-expand test.cpp
+;
+; extern void use(int);
+; extern int foo(long long);
+; 
+; void test(int p)
+; {
+;   int i = p + 4;
+;   (void)foo(i);  // sign extension of i
+;   if (p)
+;     return;
+;   use(i);        // use of original i
+; }
+;
+; PR39845: Check that CodeGenPrepare does not drop the reference to a local when it is
+;          sign-extended and used later.
+;
+; CHECK: define{{.*}}test
+; CHECK: call{{.*}}dbg.value(metadata i32 %p
+; CHECK: call{{.*}}dbg.value(metadata i32 %add
+;
+define dso_local void @_Z4testi(i32 %p) local_unnamed_addr !dbg !7 {
+entry:
+  call void @llvm.dbg.value(metadata i32 %p, metadata !12, metadata !DIExpression()), !dbg !14
+  %add = add nsw i32 %p, 4, !dbg !15
+  call void @llvm.dbg.value(metadata i32 %add, metadata !13, metadata !DIExpression()), !dbg !16
+  %conv = sext i32 %add to i64, !dbg !17
+  %call = tail call i32 @_Z3foox(i64 %conv), !dbg !18
+  %tobool = icmp eq i32 %p, 0, !dbg !19
+  br i1 %tobool, label %if.end, label %cleanup, !dbg !21
+
+if.end:
+  tail call void @_Z3usei(i32 %add), !dbg !22
+  br label %cleanup, !dbg !23
+
+cleanup:
+  ret void, !dbg !23
+}
+
+declare dso_local i32 @_Z3foox(i64) local_unnamed_addr
+declare dso_local void @_Z3usei(i32) local_unnamed_addr
+
+declare void @llvm.dbg.value(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 348209)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: "/home/test/src")
+!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 348209)"}
+!7 = distinct !DISubprogram(name: "test", linkageName: "_Z4testi", scope: !1, file: !1, line: 4, type: !8, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null, !10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!12, !13}
+!12 = !DILocalVariable(name: "p", arg: 1, scope: !7, file: !1, line: 4, type: !10)
+!13 = !DILocalVariable(name: "i", scope: !7, file: !1, line: 6, type: !10)
+!14 = !DILocation(line: 4, column: 15, scope: !7)
+!15 = !DILocation(line: 6, column: 13, scope: !7)
+!16 = !DILocation(line: 6, column: 7, scope: !7)
+!17 = !DILocation(line: 7, column: 13, scope: !7)
+!18 = !DILocation(line: 7, column: 9, scope: !7)
+!19 = !DILocation(line: 8, column: 7, scope: !20)
+!20 = distinct !DILexicalBlock(scope: !7, file: !1, line: 8, column: 7)
+!21 = !DILocation(line: 8, column: 7, scope: !7)
+!22 = !DILocation(line: 10, column: 3, scope: !7)
+!23 = !DILocation(line: 11, column: 1, scope: !7)