[NOT FOR COMMIT(yet)] [deadargelim] Use entry values for unused args

[djtodoro]

Summary

Currently we generate entry value only within LiveDebugValues, but it seems we can use it within DeadArgElim for unused arguments.

Addresses PR39716.

[dblaikie]

llvm/lib/CodeGen/LiveDebugValues.cpp

1843–1853

Hmm - how do we use entry values, if we don't know what parts of the parameter's range are valid? Does this only recover entry values for descriptions that are about to get clobbered here? Not for descriptions that became dead/were removed in previous optimization passes?

(yeah, guess that means we're missing a lot of entry value use, eg:

void f1(int);
void f2(int i) {
  i = i + 5; // line 3
  f1(3);
}

We get an entry value if line 3 is deleted, but otherwise we don't get one/only get a location that's valid up until the 'f1' call.
Similarly if line 3 was int j = i + 5; an entry_value location could be used to describe that but isn't. Oh, even if "int j = i;" is used, that doesn't get an entry value location.

I guess overall my point would be we should be creating entry_values a lot more in intermediate passes.

Hmm, I wonder if we'd ever need to combine these strategies - but I guess not. If a location is already described with an entry value, no need to use another. Oh, if we do end up with the work to describe a variable's value with multiple IR Registers - then we might want this. ALso if we are able to use entry_value not at the top-level, then maybe this patch is incorrect? (maybe leave a FIXME about generalizing this for those cases? Could write IR test cases, well at least for the non-top-level: an entry_value + 5, for instance?kEven if no pass is creating such a thing right now, it's probably good/reasonable to support it at the IR level?)

[djtodoro]

llvm/lib/CodeGen/LiveDebugValues.cpp

1843–1853

Actually, this piece of code here (on the line 1852) is unwanted (maybe), I need to test it.

In general, entry_value depends on call-site info (call_value attribute of corresponding call-site-param). We generate entry_values in the callee as a backup locations, and we don't know whether it will turn into real value. The more call-site-parameters the more of these will be real values.
Currently, we generate entry_values *only* for unmodified parameters. We can use entry_value op for modified params as well, if the modification could be expressed in terms of its entry value. There are TODOs within isEntryValueCandidate(). I remember I tried to add support for that case here (D68973), but it was quick-made-patch, and I've never returned to it. My opinion is that such support would be easier for the implementation on the IR level, and we might want to implement it there.

[dblaikie]

llvm/lib/CodeGen/LiveDebugValues.cpp

1843–1853

Actually, this piece of code here (on the line 1852) is unwanted (maybe), I need to test it.

OK - not sure I understand why it may or may not be wanted, though.

In general, entry_value depends on call-site info (call_value attribute of corresponding call-site-param). We generate entry_values in the callee as a backup locations, and we don't know whether it will turn into real value. The more call-site-parameters the more of these will be real values.

Yep, I'm with you there.

Currently, we generate entry_values *only* for unmodified parameters. We can use entry_value op for modified params as well, if the modification could be expressed in terms of its entry value.

We should be able to use them for non-parameters as well, eg:

void f1(int i) {
  int j = i;
  i = 7;
}

There are TODOs within isEntryValueCandidate(). I remember I tried to add support for that case here (D68973), but it was quick-made-patch, and I've never returned to it. My opinion is that such support would be easier for the implementation on the IR level, and we might want to implement it there.

I'd have thought it'd be fairly generalized - usable at the IR level, but with the CodeGen level here used for cases where the code generation truncates a live range & we could fall back to an entry value. Though I suppose to do that we need to know that an entry value is usable for the given variable - which means tracking potentially two locations throughout the IR. ("here's the main location and here's an entry_value-based location you can use if you end up truncating or otherwise clobbering/shrinking the live range of the main location")

[djtodoro]

llvm/lib/CodeGen/LiveDebugValues.cpp

1843–1853

Actually, this piece of code here (on the line 1852) is unwanted (maybe), I need to test it.

OK - not sure I understand why it may or may not be wanted, though.

We don't support fragmented entry values, so it is better to leave like this.

In general, entry_value depends on call-site info (call_value attribute of corresponding call-site-param). We generate entry_values in the callee as a backup locations, and we don't know whether it will turn into real value. The more call-site-parameters the more of these will be real values.

Yep, I'm with you there.

Currently, we generate entry_values *only* for unmodified parameters. We can use entry_value op for modified params as well, if the modification could be expressed in terms of its entry value.

We should be able to use them for non-parameters as well, eg:

void f1(int i) {
  int j = i;
  i = 7;
}

Defensively! Local variables may gain benefits as well.

There are TODOs within isEntryValueCandidate(). I remember I tried to add support for that case here (D68973), but it was quick-made-patch, and I've never returned to it. My opinion is that such support would be easier for the implementation on the IR level, and we might want to implement it there.

I'd have thought it'd be fairly generalized - usable at the IR level, but with the CodeGen level here used for cases where the code generation truncates a live range & we could fall back to an entry value. Though I suppose to do that we need to know that an entry value is usable for the given variable - which means tracking potentially two locations throughout the IR. ("here's the main location and here's an entry_value-based location you can use if you end up truncating or otherwise clobbering/shrinking the live range of the main location")

Yes, something similar we did in the latest implementation within LiveDebugValues. Do you think it could be a separate IR pass (e.g. EntryValuePass) or (somehow) to extend variable's metadata and in every moment of the pipeline we pick the backup/entry_value when the primary location/value has been lost?

[dblaikie]

llvm/lib/CodeGen/LiveDebugValues.cpp

1843–1853

There are TODOs within isEntryValueCandidate(). I remember I tried to add support for that case here (D68973), but it was quick-made-patch, and I've never returned to it. My opinion is that such support would be easier for the implementation on the IR level, and we might want to implement it there.

I'd have thought it'd be fairly generalized - usable at the IR level, but with the CodeGen level here used for cases where the code generation truncates a live range & we could fall back to an entry value. Though I suppose to do that we need to know that an entry value is usable for the given variable - which means tracking potentially two locations throughout the IR. ("here's the main location and here's an entry_value-based location you can use if you end up truncating or otherwise clobbering/shrinking the live range of the main location")

Yes, something similar we did in the latest implementation within LiveDebugValues. Do you think it could be a separate IR pass (e.g. EntryValuePass) or (somehow) to extend variable's metadata and in every moment of the pipeline we pick the backup/entry_value when the primary location/value has been lost?

What I was picturing was extending the variable location metadata to describe two locations - eg: add an extra two-state enum parameter that could be used to encode entry_value-relative locations separate from/even overlapping with the non-conditional locations.

That way if a non-conditional location is cut short, we'd have the entry_value location to fallback on in some cases.

This is, of course, a very invasive change & not something to be done lightly (usual: if you're interested let's chat about this with the usual folks on llvm-dev, get some worked examples, etc)- but seems to me like what would be needed to get the most out of entry_values - maybe other folks have other ideas/directions, though.

[djtodoro]

llvm/lib/CodeGen/LiveDebugValues.cpp

1843–1853

There are TODOs within isEntryValueCandidate(). I remember I tried to add support for that case here (D68973), but it was quick-made-patch, and I've never returned to it. My opinion is that such support would be easier for the implementation on the IR level, and we might want to implement it there.

I'd have thought it'd be fairly generalized - usable at the IR level, but with the CodeGen level here used for cases where the code generation truncates a live range & we could fall back to an entry value. Though I suppose to do that we need to know that an entry value is usable for the given variable - which means tracking potentially two locations throughout the IR. ("here's the main location and here's an entry_value-based location you can use if you end up truncating or otherwise clobbering/shrinking the live range of the main location")

Yes, something similar we did in the latest implementation within LiveDebugValues. Do you think it could be a separate IR pass (e.g. EntryValuePass) or (somehow) to extend variable's metadata and in every moment of the pipeline we pick the backup/entry_value when the primary location/value has been lost?

What I was picturing was extending the variable location metadata to describe two locations - eg: add an extra two-state enum parameter that could be used to encode entry_value-relative locations separate from/even overlapping with the non-conditional locations.

That way if a non-conditional location is cut short, we'd have the entry_value location to fallback on in some cases.

This is, of course, a very invasive change & not something to be done lightly (usual: if you're interested let's chat about this with the usual folks on llvm-dev, get some worked examples, etc)- but seems to me like what would be needed to get the most out of entry_values - maybe other folks have other ideas/directions, though.

1843–1853

Sure, we should be using entry_values more, even on IR in order to save variables locations/values. One possible solution may be extending DBG_VALUE to track the backup, so we can easily track modifications that could be expressed in terms of entry_values, but we'll see... I'll be away from keyboard for two weeks, but when I am back, I'll try out some possibilities and send/start the conversation on this topic on llvm-dev. Thanks!

llvm/lib/CodeGen/LiveDebugValues.cpp

bool LiveDebugValues::ExtendRanges(MachineFunction &MF) {
   // looking for debug entry value candidates.
   DefinedRegsSet DefinedRegs;
 
   // Only in the case of entry MBB collect DBG_VALUEs representing
   // function parameters in order to generate debug entry values for them.
   MachineBasicBlock &First_MBB = *(MF.begin());
   for (auto &MI : First_MBB) {
     collectRegDefs(MI, DefinedRegs, TRI);
-      if (MI.isDebugValue())
-        recordEntryValue(MI, DefinedRegs, OpenRanges, VarLocIDs);
+    // We might already have generated the entry values within deadargelim,
+    // so we don't consider these as candidates for entry values anymore.
+    if (MI.isDebugValue() && !MI.isDebugEntryValue())
   }
 
   // Initialize per-block structures and scan for fragment overlaps.
   for (auto &MBB : MF)
     for (auto &MI : MBB)
-      if (MI.isDebugValue())
+      if (MI.isDebugValue() && !MI.isDebugEntryValue())
         accumulateFragmentMap(MI, SeenFragments, OverlapFragments);
 
   auto hasNonArtificialLocation = [](const MachineInstr &MI) -> bool {
     if (const DebugLoc &DL = MI.getDebugLoc())
       return DL.getLine() != 0;
     return false;
   };
   for (auto &MBB : MF)
     if (none_of(MBB.instrs(), hasNonArtificialLocation))

llvm/lib/IR/Verifier.cpp

 }
 
 void Verifier::verifyNotEntryValue(const DbgVariableIntrinsic &I) {
   DIExpression *E = dyn_cast_or_null<DIExpression>(I.getRawExpression());
 
   // We don't know whether this intrinsic verified correctly.
   if (!E || !E->isValid())
     return;
-
-  AssertDI(!E->isEntryValue(), "Entry values are only allowed in MIR", &I);
 }
 
 void Verifier::verifyCompileUnits() {
   // When more than one Module is imported into the same context, such as during
   // an LTO build before linking the modules, ODR type uniquing may cause types
   // to point to a different CU. This check does not make sense in this case.
   if (M.getContext().isODRUniquingDebugTypes())
     return;

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

 
 #include "llvm/Transforms/IPO/DeadArgumentElimination.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/IR/Argument.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Constants.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/NoFolder.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/IR/Type.h"
 #include "llvm/IR/Use.h"
 #include "llvm/IR/User.h"
 #include "llvm/IR/Value.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
+#include "llvm/Transforms/Utils/Local.h"
 #include <cassert>
 #include <cstdint>
 #include <utility>
 #include <vector>
 
 using namespace llvm;
 
 #define DEBUG_TYPE "deadargelim"
bool DeadArgumentEliminationPass::RemoveDeadArgumentsFromCallers(Function &Fn) {
 
   SmallVector<unsigned, 8> UnusedArgs;
   bool Changed = false;
 
   for (Argument &Arg : Fn.args()) {
     if (!Arg.hasSwiftErrorAttr() && Arg.use_empty() &&
         !Arg.hasPassPointeeByValueCopyAttr()) {
       if (Arg.isUsedByMetadata()) {
-        Arg.replaceAllUsesWith(UndefValue::get(Arg.getType()));
+        // Create entry values for unused args.
+        SmallVector<DbgValueInst *, 1> DbgValues;
+        findDbgValues(DbgValues, &Arg);
+        for (auto *DVI : DbgValues) {
+          DIExpression *NewExpr = DIExpression::prepend(
+              DVI->getExpression(), DIExpression::EntryValue);
+          DVI->setArgOperand(
+              2, MetadataAsValue::get(Arg.getType()->getContext(), NewExpr));
+        }
+
         Changed = true;
       }
       UnusedArgs.push_back(Arg.getArgNo());
     }
   }
 
   if (UnusedArgs.empty())
     return false;

llvm/test/Transforms/DeadArgElim/dbginfo-update-dbgval.ll

 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 
 @s = common dso_local local_unnamed_addr global i32 0, align 4, !dbg !0
 
 ; Function Attrs: noinline nounwind uwtable
 define dso_local void @f2(i32 %k) local_unnamed_addr !dbg !11 {
 entry:
-; CHECK: call void @llvm.dbg.value(metadata i32 undef, metadata !15, metadata !DIExpression()), !dbg !16
+; CHECK: call void @llvm.dbg.value(metadata i32 %k, metadata !15, metadata !DIExpression(DW_OP_LLVM_entry_value, 1)), !dbg !16
   call void @llvm.dbg.value(metadata i32 %k, metadata !15, metadata !DIExpression()), !dbg !16
   %0 = load i32, i32* @s, align 4, !dbg !17
   %inc = add nsw i32 %0, 1, !dbg !17
   store i32 %inc, i32* @s, align 4, !dbg !17
   call void @llvm.dbg.value(metadata i32* @s, metadata !15, metadata !DIExpression(DW_OP_deref)), !dbg !16
   ret void, !dbg !18
 }
 

llvm/test/Verifier/diexpression-entry-value-llvm-ir.ll

-; RUN: llvm-as -disable-output <%s 2>&1| FileCheck %s
-
-; The DW_OP_LLVM_entry_value operation can only be used in MIR.
-
-; CHECK: Entry values are only allowed in MIR
-; CHECK: call void @llvm.dbg.value(metadata i32 %param, metadata !{{.*}}, metadata !DIExpression(DW_OP_LLVM_entry_value, 1))
-; CHECK: warning: ignoring invalid debug info
-
-define void @foo(i32 %param) !dbg !4 {
-entry:
-  call void @llvm.dbg.value(metadata i32 %param, metadata !8, metadata !DIExpression(DW_OP_LLVM_entry_value, 1)), !dbg !9
-  ret void
-}
-
-declare void @llvm.dbg.value(metadata, metadata, metadata) #0
-
-attributes #0 = { nounwind readnone speculatable willreturn }
-
-!llvm.dbg.cu = !{!0}
-!llvm.module.flags = !{!2, !3}
-
-!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, emissionKind: FullDebug)
-!1 = !DIFile(filename: "a.c", directory: "/")
-!2 = !{i32 2, !"Dwarf Version", i32 4}
-!3 = !{i32 2, !"Debug Info Version", i32 3}
-!4 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, type: !5, unit: !0)
-!5 = !DISubroutineType(types: !6)
-!6 = !{null, !7}
-!7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
-!8 = !DILocalVariable(name: "param", arg: 1, scope: !4, file: !1, type: !7)
-!9 = !DILocation(line: 0, scope: !4)