(@labath Sorry for late response, I've been away from keyboard for some time.)

some comments inline, lgtm otherwise

Ping. :)

Hi @xiangzhangllvm, please wait for @vsk to approve as well, since he is author of the original debugify IR patch and he is very involved in the area.
Thanks a lot for your patience and work.

In D91722#2431127, @StephenTozer wrote:

In D91722#2404937, @djtodoro wrote:

Thanks!! So, this should be the most important one :) I would really like to see some measurements (e.g. llvm-locstats) before/after this patch.
Have you tried building llvm-project itself with this?

So far I've tested this with the benchmarks from the llvm test suite (specifically the CTMark projects, since we're looking for compilation metrics only). I'll post a more complete set of stats soon, but the summary is that there is a small increase in compile time for most projects; <1% or outright negligible in most cases. In terms of locstats, the change looks to add a modest 1-2% increase in overall variable coverage for most projects, although a notable exception is tramp3d-v4 which saw variable coverage change from 53%=>66% - a fairly substantial improvement.

I'm optimistic about further improvements to the locstats following from this work; the current patch doesn't update any of the existing salvaging in Instruction Selection to support variadic locations (they require some specific changes which aren't complicated but require a separate patch), and also enable some new salvaging opportunities in Instruction Selection and the loop passes (such as Loop Strength Reduce).

LGTM, some nits inline. Thanks.

lgtm, thanks!

Thanks for addressing the comments.

lgtm, but please keep just one test case, since it is enough (as @dstenb suggested)

In D92471#2428505, @dstenb wrote:

I think it would be preferable if we could do this in a target independent place, so that downstream targets, and upstream targets that do not yet support call sites, do not have to care about this.

Can we do this in collectCallSiteParameters by looking at the call instruction's undef uses?

Something like this:

@@ -787,6 +787,15 @@ static void collectCallSiteParameters(const MachineInstr *CallMI,
     (void)InsertedReg;
   }
 
+  for (auto &MO : CallMI->uses()) {
+    if (!MO.isReg() || !MO.isUndef())
+      continue;
+    auto It = ForwardedRegWorklist.find(MO.getReg());
+    if (It == ForwardedRegWorklist.end())
+      continue;
+    ForwardedRegWorklist.erase(It);
+  }
+

This makes sense to me. The call site info represents info about param value transferring, so if the value was undef, we have "nothing to transfer".

• Use the entry values for local vars as well

• Support old metadata
• Fix testcases

(please leave some time (a few days) to other reviewers to take a look)

Thanks for addressing the comments! We are almost there.

Nice! (few nits inline)

Thanks!

Thanks!! So, this should be the most important one :) I would really like to see some measurements (e.g. llvm-locstats) before/after this patch.
Have you tried building llvm-project itself with this?

Thanks for doing this! (some initial comments inline)

OK, IMPO we can go with removing it completely from the source.

lgtm, thanks.

Super nit inline.

@jmorse Thanks. :)

LGTM; Please remove "Entry values" from the top-level comment TODO list.

It was huge. Thanks.

This looks reasonable to me.

@StephenTozer Thanks for your comments. I'll address them and refactor the patch a bit, as soon as I catch some time to address some issues with the first patch from the stack.

Some nits included.

In D87233#2321073, @djtodoro wrote:

In D87233#2316204, @dstenb wrote:

If I understand this correctly, the new {EntryValue, EntryExpr} operands do, if EntryExpr is not undef, specify a location that is identical to the dbg.value's current {Value, Expr} operands, but with DW_OP_LLVM_entry_value implicitly being applied to EntryValue before EntryExpr. Is that correct?

Yes, that is correct.

If so, just throwing out some questions (sorry in case I have overlooked the answers for this in the implementation):

• I assume that we are only interested in EntryValue operands that are Argument values? Should the verifier catch that? Or is there some case where it could anything else than an Argument?

I think we should extend the Verifier with some kind of support for this. Not sure at the moment how we are going to handle this:

fn(%param) {
...
}

isa<Argument>(%param) ==>true
isa<Argument>(%param.something) ==> false (something could be `addr`)

Actually, I extended the Verifier locally, and I am catching some cases during the replaceAllUsesWith() where the entry value needs to be handled differently, e.g.:

llvm.dbg.value(%p1, !var_p1, !DIExpr(), %p1, !DIExpr()) becomes llvm.dbg.value(i32 0, !var_p1, !DIExpr(), i32 0, !DIExpr())

but we want the entry value to be an undef in this situation, so I am wondering whether we need a new representation of the entry-value, something like MetadataAsEntryValue, so we can invalidate such nodes in an efficent way.

• What should happen in cases where the dbg.values Value operand already is an Argument? Do we need to specify that value in the EntryValue operand also to ensure that entry values can be emitted?

We should, since the Argument (from some point) can start depending on different entry value, please consider this case:

void fn (int p1, int p2) {
   ...
   p1 = p2;
}

And the intrinsics would be:

llvm.dbg.value(%p1, !var_p1, !DIExpr(), %p1, !DIExpr())
llvm.dbg.value(%p2, !var_p1, !DIExpr(), %p2, !DIExpr())

!var_p2, sorry for my mistake

....
llvm.dbg.value(%p1, !var_p1, !DIExpr(), %p2, !DIExpr()) <=== p1 is an argument, but it depends on different entry value

In D87233#2316204, @dstenb wrote:

If I understand this correctly, the new {EntryValue, EntryExpr} operands do, if EntryExpr is not undef, specify a location that is identical to the dbg.value's current {Value, Expr} operands, but with DW_OP_LLVM_entry_value implicitly being applied to EntryValue before EntryExpr. Is that correct?

Yes, that is correct.

TODO: Handle Function Inlining

• Use VMContext instead of InsertBB->getParent()->getContext()

• Avoid null metadata within the new llvm.dbg.value()

This needs to be tested more (so it is not for the final commit (yet)). I believe this triggers some asserts, but (at least) we are covering the cases we want.

• Use 2 extra arguments from llvm.dbg.value()
• Handle modified params

• Extend the llvm.dbg.value() with 2 operands by describing Entry Value

I am about to post new set of patches addressing this problem. We are back to the initial idea that refers to extending the llvm.dbg.value() to hold info about the entry Value. We need to do that, since the current approach (by relying on the method that uses the DILocalVariable*) couldn't detect variable modifications (in an efficient way). Another advantage we are getting with this new approach is that we can handle local variables as well, if it can be expressed in terms of an entry value such as:

void fn (int param) {
 ...
 int local = param + 2;
 ...

• Return the names

mostly lgtm, some initinal comments inline, thanks!

Thanks a lot for such a nice explanation! I've got it (at least for this example)!

Hi @jmorse, cool, nice improvement! It turns out that this value-based value tracking pass makes it simple as well as effective!

-Rebasing

-Rebasing

-Addressing the latest comments
-Rebasing

@StephenTozer @dstenb Thanks for your comments! I'll try to change the approach (by putting some extra context), so we might end up changing this one (a bit).