[ArgumentPromotion] Change use of removed argument in llvm.dbg.value to undef

[uabelho]

Summary

This solves PR33641.

When removing a dead argument we must also handle possibly existing calls
to llvm.dbg.value that use the removed argument. Now we change the use
of the otherwise dead argument to an undef for some other pass to cleanup
later.

If the calls are left untouched, they will later on cause errors:
"function-local metadata used in wrong function"
since the ArgumentPromotion rewrites the code by creating a new function
with the wanted signature, but the metadata is not recreated so the new
function may then erroneously use metadata from the old function.

[uabelho]

Not sure who should review this so I added the last three persons who changed ArgumentPromotion.cpp

[rnk]

lib/Transforms/IPO/ArgumentPromotion.cpp

134

There's no reason to believe the only uses of the argument will appear in the entry block. I think you really want to do I->replaceAllUsesWith(undef) to replace all metadata uses of the argument. Some other pass (instcombine?) can clean up the useless dbg.value instructions. That's what the dead argument elimination pass does, anyway.

[uabelho]

Changed according to rnk's comments.

Thanks!

[rnk]

lgtm

lib/Transforms/IPO/ArgumentPromotion.cpp

128

This shorter comment should suffice:

// There may be remaining metadata uses of the argument for things like llvm.dbg.value. Replace them with undef.

I think the reason this is special is that for some reason the Function isn't being deleted (is it leaked?), and the Argument isn't being deleted, which would trigger removal from the ValuesAsMetadata map.

[uabelho]

Updated code comment.

[uabelho]

Thanks! I'll submit this now then.

lib/Transforms/IPO/ArgumentPromotion.cpp

if (ByValArgsToTransform.count(&*I)) {
       ++NumByValArgsPromoted;
     } else if (!ArgsToPromote.count(&*I)) {
       // Unchanged argument
       Params.push_back(I->getType());
       ArgAttrVec.push_back(PAL.getParamAttributes(ArgNo));
     } else if (I->use_empty()) {
       // Dead argument (which are always marked as promotable)
       ++NumArgumentsDead;
+
+      // There may be remaining metadata uses of the argument for things like
+      // llvm.dbg.value. Replace them with undef.
+      I->replaceAllUsesWith(UndefValue::get(I->getType()));
     } else {
       // Okay, this is being promoted. This means that the only uses are loads
       // or GEPs which are only used by loads
 
       // In this table, we will track which indices are loaded from the argument
       // (where direct loads are tracked as no indices).
       ScalarizeTable &ArgIndices = ScalarizedElements[&*I];
       for (User *U : I->users()) {
         Instruction *UI = cast<Instruction>(U);
         Type *SrcTy;
         if (LoadInst *L = dyn_cast<LoadInst>(UI))
           SrcTy = L->getType();

test/Transforms/ArgumentPromotion/pr33641_remove_arg_dbgvalue.ll

+; RUN: opt -argpromotion -verify -dse -S %s -o - | FileCheck %s
+
+; Fix for PR33641. ArgumentPromotion removed the argument to bar but left the call to
+; dbg.value which still used the removed argument.
+
+%p_t = type i16*
+%fun_t = type void (%p_t)*
+
+define void @foo() {
+  %tmp = alloca %fun_t
+  store %fun_t @bar, %fun_t* %tmp
+  ret void
+}
+
+define internal void @bar(%p_t %p)  {
+  call void @llvm.dbg.value(metadata %p_t %p, i64 0, metadata !4, metadata !5), !dbg !6
+  ret void
+}
+
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1)
+!1 = !DIFile(filename: "test.c", directory: "")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = distinct !DISubprogram(name: "bar", unit: !0)
+!4 = !DILocalVariable(name: "p", scope: !3)
+!5 = !DIExpression()
+!6 = !DILocation(line: 1, column: 1, scope: !3)
+
+; The %p argument should be removed, and the use of it in dbg.value should be
+; changed to undef.
+; CHECK:      define internal void @bar() {
+; CHECK-NEXT:   call void @llvm.dbg.value(metadata i16* undef
+; CHECK-NEXT:   ret void
+; CHECK-NEXT: }