[LiveDebugValues] Introduce entry values of unmodified params

[djtodoro]

Summary

The idea is to remove front-end analysis for the parameter's value modification and leave it to the value tracking system. Front-end in some cases mark a parameter as modified even the line of code that modifies the parameter gets optimized, that implies that this will cover more entry values even for unmodified parameters (GCC generates entry values in the situations like that, because it performs such analysis within its own value tracking system). In addition, extending the support for modified parameters will be easier with this approach.

Since the goal is to recognize if a parameter’s value has changed, the idea at very high level is: If we encounter a DBG_VALUE other than the entry value one describing the same variable (parameter), we can assume that the variable’s value has changed and we should not track its entry value any more. That would be ideal scenario, but due to various LLVM optimizations, a variable’s value could be just moved around from one register to another (and there will be additional DBG_VALUEs describing the same variable), so we have to recognize such situation (otherwise, we will lose a lot of entry values) and salvage the debug entry value.

[djtodoro]

This is very first (kind of a template) patch and it should be tested and improved more, but the main point is that this will remove the performance regression and improve the debug info quality. :)

[aprantl]

So we've come full circle :-)
I'm not a fan of lumping arbitrary funcitonality into LiveDebugValues, but if we can keep it readable this is probably the best place for it.

llvm/lib/IR/DebugInfoMetadata.cpp

966 ↗

(On Diff #222392)

This appears to be of limited use since many DIExpressions are more complex. Is this meant to detect spill locations? If yes, could we mark them differently when we create them instead of trying to detect them here?

[jmorse]

Hi Djordje,

Djordje wrote:

[...] due to various LLVM optimizations, a variable’s value could be just moved around from one register to another (and there will be additional DBG_VALUEs describing the same variable), so we have to recognize such situation (otherwise, we will lose a lot of entry values) and salvage the debug entry value.

The chain of patches I have in D67393 / D67398 / D67500 could be useful here -- the DBG_VALUEs that LiveDebugValues inserts for transfers can cause other problems, and in those patches I've attempted to delay their creation until after LiveDebugValues is finished. In particular, from the summary of D67398,

I wrote:

previously VarLocs could effectively build long chains where the MI field referred to the previous transfer DBG_VALUE, eventually rooted in a [non-transfer] DBG_VALUE. Now, however, VarLocs can only ever refer to the [non-transfer] DBG_VALUE, which I think is slightly neater

This means that whenever a transfer is created by transferRegisterCopy, no DBG_VALUE is created, instead a VarLoc is created that points back to the "original" DBG_VALUE. If I understand you correctly (75% confidence), with those patches you would be able to identify whether a location is the entry value by examining the VarLoc's MI field, even if the location has been transferred in the meantime.

[djtodoro]

@aprantl

So we've come full circle :-)

Unfortunately, yes. :)

I'm not a fan of lumping arbitrary funcitonality into LiveDebugValues, but if we can keep it readable this is probably the best place for it.

+1

@jmorse

Hi Jeremy,

I think as well the set of patches you applied could be useful here, I see 2/3 are already committed and I will rebase this on top of that. :)

If I understand you correctly (75% confidence), with those patches you would be able to identify whether a location is the entry value by examining the VarLoc's MI field, even if the location has been transferred in the meantime.

Yes, that is the goal.

llvm/lib/IR/DebugInfoMetadata.cpp

966 ↗

(On Diff #222392)

I am aware of that, we should definitely make it more useful somehow.

The point is that this is meant to be used to detect modification of a parameter that could be expressed in terms of its entry value. Up to this point we supported only unmodified parameters, but at this point we want to extend it and try to salvage entry values even for modified parameters (as GCC does). The problem is that we are analyzing only DBG_VALUEs in order to distinguish whether a parameter's value has changed and if so, the idea is only to look at the DIExpression to distinguish whether to track the modification (e.g. expressions like plus_const, minus_const, etc.) or not, by not analyzing the actual MI that performs the modification that the DBG_VALUE describes. In addition, the problem is in many cases the instruction performing the parameter's modification gets optimized even at IR level, and we have just the DBG_VALUE describing that modification. Consider the test case attached:

...
b = b + 7;
...

the add intrinsic will be removed after the Combine redundant instructions pass and we will have just the dbg.value() at the place describing actual modification, but we want to generate the expression like DW_OP_LLVM_entry_value, 1, DW_OP_plus_uconst, 7, DW_OP_stack_value in LiveDebugValues.

I think that I should split this patch into support for unmodified and modified parameters, and that will be easier to review.

[djtodoro]

-Split this into unmodifed/modified parameters support
-Clean the code a bit
-Rebasing on the top of the latest changes in the pass

[djtodoro]

With the new approach, by building the GDB 7.11 we confirm that there is no performance regression any more (by using the debug entry values experimental option).

		-g -O2		-g -O2 -Xclang -femit-debug-entry-values (old approach)		-g -O2 -Xclang -femit-debug-entry-values (new approach)
build time		355 seconds		524 seconds		358 seconds

By using the llvm-locstats tool, we confirm that the improvement in terms of debug location coverage is still there.

-The stats from gdb compiled with -g -O2:
llvm-locstats -only-formal-parameters gdb

==================================================
              Debug Location Statistics
==================================================
        cov%           samples       percentage(~)
--------------------------------------------------
        0%               5533            7%
        1-9%             2889            4%
        10-19%           3149            4%
        20-29%           2696            3%
        30-39%           2545            3%
        40-49%           2380            3%
        50-59%           2311            3%
        60-69%           2262            3%
        70-79%           2467            3%
        80-89%           3080            4%
        90-99%          10140            14%
        100%            29982            43%
================================================
-the number of debug variables processed: 69434
-PC ranges covered: 64%
================================================

-The stats from gdb with entry values (new approach):
llvm-locstats -only-formal-parameters gdb

==================================================
              Debug Location Statistics
==================================================
        cov%           samples       percentage(~)
--------------------------------------------------
        0%               5527            7%
        1-9%             1854            2%
        10-19%           2161            3%
        20-29%           1731            2%
        30-39%           1642            2%
        40-49%           1556            2%
        50-59%           1546            2%
        60-69%           1596            2%
        70-79%           1805            2%
        80-89%           2466            3%
        90-99%           6437            9%
        100%            41113            59%
================================================
-the number of debug variables processed: 69434
-PC ranges covered: 73%
================================================

[djtodoro]

llvm/lib/CodeGen/LiveDebugValues.cpp

790

This is only NFC. This can go as a separate commit, since this piece of code is not used any more.

[dstenb]

llvm/lib/CodeGen/LiveDebugValues.cpp

94–99

As a performance optimization, perhaps consider an early exit like this:

if (!Op.isReg())
  return false;

to avoid running the query functions if not needed.

273

seting -> setting

1142

In order to improve the readability, can you move this into a helper function?

[dstenb]

llvm/lib/CodeGen/LiveDebugValues.cpp

753

Deliting -> Deleting

[djtodoro]

@dstenb Thanks a lot for your comments!

-Addressing comments

[vsk]

Hi @djtodoro, thanks for the patch as always. I'm curious about the DbgEntryValueMovement field. Could you explain it in a little more detail? At a high level, IIUC, it's used to mark copies of entry values for the purpose of simplifying location lists (i.e. keep an original AT_entry_value location description even if there are copies). Is that correct? Does the approach handle there being multiple copies of an entry value within a block?

llvm/lib/CodeGen/LiveDebugValues.cpp

727

Why is the const_cast needed?

831

It would be good to simplify this expression, possibly by sinking it into a helper function that returns a VarLoc.

1127

Why move the DestRegOp->isDef() check to after the isCalleSavedReg lambda definition?

1154

auto &EntryValue?

[djtodoro]

@vsk Thanks for the comments! Sure, I will try. :)

The idea from very high level, in this patch, is to generate entry values (when needed) for unmodified parameters (for parameters whose value has not changed throughout a function), by examining the code in at this stage of LLVM pipeline (we removed the front-end part due to performance regression). The main premise here is that the parameter’s value has changed if we encounter a new DBG_VALUE within a block describing the same parameter, and if so we stop tracking the entry value of such parameter. Nevertheless, there are many cases where (due to various optimizations) a parameter value is only moved (copied) around, from one register to another one. We should recognize such situation and keep tracking of entry value of such parameter, since the value actually has not changed. For that scenario, we use the DbgEntryValueMovement field to remember such movement and use it when creating the VarLoc for such entry value. This should handle multiple copies within a block.

llvm/lib/CodeGen/LiveDebugValues.cpp

727

I have use it for downstream code purposes, it was mistake keeping it here... Thanks!

1127

Mistake.

1154

We change the EntryValue with the setDbgEntryValueMovement().

[djtodoro]

-Addressing comments

[jmorse]

Hi,

Djordje wrote:

Nevertheless, there are many cases where (due to various optimizations) a parameter value is only moved (copied) around, from one register to another one. We should recognize such situation and keep tracking of entry value of such parameter, since the value actually has not changed.

A large number of these cases Should (TM) have gone away with D67393, as any transfer DBG_VALUEs created by LiveDebugValues don't enter the instruction stream until after LiveDebugValues completes. In fact, applying the two parent patches and the test from this change to master (which I've done in this tree here [0]), the test already passes for me. (There's a decent chance I mucked something up though).

However, the test case demonstrates something I've found odd in LLVM instruction selection: it creates _two_ DBG_VALUEs for arguments. The code that does it is here [1], dates back to about 2010, and sounds like it tries to propagate variable locations in a way that is now LiveDebugValues domain. If it didn't create an extra DBG_VALUE for every argument, we might not need special handling for extra DBG_VALUEs here. Removing the code in [1] makes very little difference to variable-location coverage statistics, but does trip 12 tests.

I don't have any specific feedback on this patch, but I get the feeling that it might not be necessary if we eliminated [1]; I don't suppose anyone remembers what it's there for?

(Sorry for driving this review in an unrelated direction, but it'd be nice if we could design-out this problem).

[0] https://github.com/jmorse/llvm-project/commits/d68209_experiment
[1] https://github.com/llvm/llvm-project/blob/f24ac13aaae63d92317dac839ce57857a7b444dc/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp#L587

llvm/lib/CodeGen/LiveDebugValues.cpp

1451

Note that this hunk didn't apply cleanly for me; on master this line (1319/1449) reads

!MI.getDebugExpression()->isFragment())

llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir

20–23

Best to not bake in the metadata numbers directly, and instead to recognise the DILocalVariable records, capture their number, and check for DBG_VALUEs with those captured numbers.

[dstenb]

llvm/lib/CodeGen/LiveDebugValues.cpp

1451

That is from D66746. That patch has been postponed a bit as I've been working on other patches, but I'll pick up that again.

[djtodoro]

Hi Jeremy,

Thanks a lot for diving into this!

However, the test case demonstrates something I've found odd in LLVM instruction selection: it creates _two_ DBG_VALUEs for arguments. The code that does it is here [1], dates back to about 2010, and sounds like it tries to propagate variable locations in a way that is now LiveDebugValues domain. If it didn't create an extra DBG_VALUE for every argument, we might not need special handling for extra DBG_VALUEs here. Removing the code in [1] makes very little difference to variable-location coverage statistics, but does trip 12 tests.

Hmm... I see. I agree that that should be the LiveDebugValues job..
Initially when I did the investigation, I have not used the D67393, but I see that it should remove large portion of the cases we care about here.

I don't have any specific feedback on this patch, but I get the feeling that it might not be necessary if we eliminated [1];

Anyhow, I think even we decide to remove the code from [1], we would need one part of this patch, to avoid some unwanted entry values generated. But, the code (and the analysis) will be away more simpler than the current one.

(Sorry for driving this review in an unrelated direction, but it'd be nice if we could design-out this problem).

I really appreciate the comments, since resolving the issue from [1] will simplify this a lot. :)

llvm/lib/CodeGen/LiveDebugValues.cpp

1451

I forgot to add a note that this relies on the D66746 as well. Sorry for that, I will update the diff.

[jmorse]

After eyeballing the code in SelectionDAG that generates the additional DBG_VALUEs, it looks like disabling that code will generate a variety of other location regressions. LiveDebugValues doesn't always pick the "best" location when there are multiple options, see for example [0].

With that in mind, this code makes sense. My understanding of what's happening here is that whenever there's a register-copy-transfer, if it's from an entry-value-location then DebugEntryVals map records the transfer destination. Then, if another DBG_VALUE for the same variable is encountered specifying the same destination, it's interpreted as being caused by the register-copy, and so entry values can be propagated over it.

I think what's missing is handling of control flow branching / merging. As far as I can see, DebugEntryVals is a single map, that gets updated whenever a block is processed. If we had a standard diamond control flow like this:

entry:
    setcc %somereg
    jnz block2
block1:
    mov %rax, %rcx
    jmp exit
block2:
    mov %rcx, %rdx
    jmp exit

Then the reverse-post-order order that LiveDebugValues follows will mean it visits blocks in the order {entry, block1, block2, exit} [1]. Isn't there a risk that changes to DebugEntryVals when processing block1, will be visible / will affect the handling of block2?

The rest of LiveDebugValues handles this by having a per-basic-block collection of in-locs / locations and updating them in the 'join' method. That would probably solve this potential issue too, but it add more complexity :(

[0] https://bugs.llvm.org/show_bug.cgi?id=42834
[1] although block1 and block2 might get flipped, I can't remember precisely.

llvm/lib/CodeGen/LiveDebugValues.cpp

278

It's probably worth asserting that you expect the incoming location to be a RegisterKind, should make it clearer for other readers.

418

Can there be fragments of entry values? Typically a <DILocalVariable, InlinedAt, Fragment> tuple is needed to uniquely identify a variable location. I understand inlining doesn't make sense in the context of entry values, but if there can be fragments, then the map key here may need to account for them, now or in the future.

720–721

"entry value if _we_ encounter"

[djtodoro]

@jmorse Thanks for the comments!

With that in mind, this code makes sense. My understanding of what's happening here is that whenever there's a register-copy-transfer, if it's from an entry-value-location then DebugEntryVals map records the transfer destination. Then, if another DBG_VALUE for the same variable is encountered specifying the same destination, it's interpreted as being caused by the register-copy, and so entry values can be propagated over it.

That is right.

I think what's missing is handling of control flow branching / merging. As far as I can see, DebugEntryVals is a single map, that gets updated whenever a block is processed. If we had a standard diamond control flow like this:
entry:
setcc %somereg
jnz block2
block1:
mov %rax, %rcx
jmp exit
block2:
mov %rcx, %rdx
jmp exit
Then the reverse-post-order order that LiveDebugValues follows will mean it visits blocks in the order {entry, block1, block2, exit} [1]. Isn't there a risk that changes to DebugEntryVals when processing block1, will be visible / will affect the handling of block2?

That could be a problem as well as tracking of clobbering of the register holding the copy of the debug entry value. I work on that as well as on a NFC that should simplify the code a bit.

[djtodoro]

llvm/lib/CodeGen/LiveDebugValues.cpp

418

At the moment, we do not support fragments, so only the DILocalVariable is enough for now.

[djtodoro]

-Make the debug entry values mapping local to the OpenRanges
-At the begging of the analysis record the entry values by making the 'backup' locations
-When needed, the backup location will turn into the primary location of a parameter
-Support diamond structure of bbs
-Add more tests

[aprantl]

I was wondering if it would make sense to let LiveDebugValues compute all available locations, and then have DbgEntityHistoryCalculator pick the best one (e.g., prefer anything over an entry value). The downside of that approach is that we'd need to reimplement some of DbgEntityHistoryCalculator in LiveDebugValues for this to work: For example whether DBG_VALUE should truncate a previous DBG_VALUE's range (e.g, if the previous one was constant) or whether it is an alternative location (such as, if a register is copied). I'm not saying that we need to do this now. But it is something we could evolve LiveDebugValues towards.

llvm/lib/CodeGen/LiveDebugValues.cpp

92

Can you adda a /// comment that explains, why this is useful / why a caller might want to know this?

[djtodoro]

I was wondering if it would make sense to let LiveDebugValues compute all available locations, and then have DbgEntityHistoryCalculator pick the best one (e.g., prefer anything over an entry value). The downside of that approach is that we'd need to reimplement some of DbgEntityHistoryCalculator in LiveDebugValues for this to work: For example whether DBG_VALUE should truncate a previous DBG_VALUE's range (e.g, if the previous one was constant) or whether it is an alternative location (such as, if a register is copied). I'm not saying that we need to do this now. But it is something we could evolve LiveDebugValues towards.

@aprantl +1. I think that should be our next step. That should simplify the code (here) a lot, and besides that, we will have the other benefits such as minimal debug location list indicating smaller .debug_loc section.

[djtodoro]

-Add the comment for the isRegOtherThanSPAndFP()

[jmorse]

This looks good -- some minor nits inline. Keeping the backup-locations in the live-out sets as special records seems like a good solution for avoiding unwanted control flow effects, as shown with the diamond pattern.

Would I be right in thinking that the OpenRangesSet::erase method will now only erase entry value backup locations if you pass in a VarLoc that's also a backup location? This seems important, so that you can call erase and insert when a location _move_ happens, without disturbing the backup value. If this is the case, it should probably go into a comment somewhere documenting why two different sets of Vars / EntryValuesBackupVars are kept, it could be missed otherwise. (If this is documented, I missed the comment, oops).

llvm/lib/CodeGen/LiveDebugValues.cpp

93

"... to avoid basing the debug entry values on the registers..." ?

362

Seeing how VarLocIDs is just a UniqueVector, isn't zero a legitimate index? I'm not aware of the first element being initialized to anything specific.

489–490

Won't having "Vars.empty()" as a clause here mean the assertion fires if the OpenRanges isn't empty, rather than testing whether it's empty?

746–748

If I understand this, if the immediately preceeding instruction is a debug instruction, it's considered to be propagated? This seems unreliable to me -- DBG_VALUEs often appear in packs.

llvm/test/DebugInfo/MIR/X86/entry-values-diamond-bbs.mir

9–14 ↗

(On Diff #227847)

IMHO more context should be checked for the entry value DBG_VALUEs, i.e. that they come after the correct instruction. The movement of "b" through these blocks is non-trivial, this test should make sure the entry values land in the right place as well as the right block. Same with the test below.

[djtodoro]

@jmorse Thanks for your comments! I tried to address them all!

llvm/lib/CodeGen/LiveDebugValues.cpp

362

I this this comments stands for getEntryValueBackup(), and I have addressed it. Thanks!

489–490

I am not sure any more, since this is not used at the current code..

746–748

Yes, I was not happy with the piece of code. I was gonna put a HACK marker above this, but I think it can be a TODO for now.

llvm/test/DebugInfo/MIR/X86/entry-values-diamond-bbs.mir

9–14 ↗

(On Diff #227847)

I agree. I have addressed this.

[djtodoro]

-Update the tests with more checking of the context
-Make more comments of the new functionality
-Address the reviewer's comments

[aprantl]

llvm/lib/CodeGen/LiveDebugValues.cpp

92

nit: /// If \p Op is a ...

[djtodoro]

-Addressing the latest (nit) comment

[djtodoro]

-Rebasing on top of the latest LLVM
-Add an additional LLVM_DEBUG() output for debugging purpose within the recordEntryValue()
-Refactor the code a bit
-Add the stack machine function attribute to a test, since it was needed for the new approach for distinguishing the callee saved regs

[jmorse]

IMO, this LGTM, with the rider that I think the "empty" method needs to be made safer (see inline).

Keeping the main / "backup" locations starts introducing a hierarchy of longevity for locations, which is where I think we want to be heading.

llvm/lib/CodeGen/LiveDebugValues.cpp

362

Reading the comments from UniqueVector.h:

/// insert - Append entry to the vector if it doesn't already exist.  Returns
/// the entry's index + 1 to be used as a unique ID.

So zero really is a special value, excellent.

489–490

I think it should probably read:

assert(Vars.empty() == EmptyValuesBackupVars.empty &&
           Vars.Empty() == VarLocs.empty() &&
           "open ranges are inconsistent");
return VarLocs.empty()

To ensure that if one is empty, all the others are. Simply asserting that Vars is empty will lead to assertions firing for completely valid states of the pass.

[djtodoro]

@jmorse Thanks for your comments!

Keeping the main / "backup" locations starts introducing a hierarchy of longevity for locations, which is where I think we want to be heading.

Thanks. I agree. We may think of refactoring this in future by using some different data structures (e.g. lists, arrays or queue) to store all valid location (primary and backups) for a variable, and try to avoid Backup kinds for every new desirable location that can occur as backup.

llvm/lib/CodeGen/LiveDebugValues.cpp

489–490

It makes sense. Thanks.

[djtodoro]

-Addressing comments

NOTE: This may be updated with a change in an LLDB test. The issue was found when committing the D68206.

[djtodoro]

When trying to test the lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values_x86_64/TestBasicEntryValuesX86_64.py I realized few things.

First of all, we are relying on the premise that whenever a source variable is set to a new value, we have a new llvm.dbg.value()[0].
By looking at the example [1], especially the code for function func1(), it looks like it is not the case here:

__attribute__((noinline))
void func1(int &sink, int x) {
  use(x);

  // Destroy 'x' in the current frame.
  DESTROY_RSI;

  //% self.filecheck("image lookup -va $pc", "main.cpp", "-check-prefix=FUNC1-DESC")
  // FUNC1-DESC: name = "x", type = "int", location = DW_OP_entry_value(DW_OP_reg4 RSI)

  ++sink;
}

the IR looks like (compiled with -g -O2):

 ; Function Attrs: noinline nounwind uwtable
define dso_local void @_Z5func1Rii(i32* nocapture dereferenceable(4) %sink, i32 %x) local_unnamed_addr #0 !dbg !14 {
entry:
  call void @llvm.dbg.value(metadata i32* %sink, metadata !19, metadata !DIExpression()), !dbg !21
  call void @llvm.dbg.value(metadata i32 %x, metadata !20, metadata !DIExpression()), !dbg !21
  tail call void @_Z3useIiEvT_(i32 %x), !dbg !22
  tail call void asm sideeffect "xorq %rsi, %rsi", "~{rsi},~{dirflag},~{fpsr},~{flags}"() #4, !dbg !23, !srcloc !24
  %0 = load i32, i32* %sink, align 4, !dbg !25, !tbaa !26
  %inc = add nsw i32 %0, 1, !dbg !25
  store i32 %inc, i32* %sink, align 4, !dbg !25, !tbaa !26
  ret void, !dbg !30
}

therefore, the resulting MIR does not have a DBG_VALUE representing the ++sink;, and with this approach, we will assume the value of the variable has not change by implying that we are able to generate the DW_OP_entry_value expression for it.

At the moment, we should avoid creating the entry value for the sink variable. Eventually, when we add support for the entry values of modified parameters, this should be ((DW_OP_GNU_entry_value: (DW_OP_reg5 (rdi)); DW_OP_plus_uconst: 1; DW_OP_stack_value)), and that is what GCC generates for it.

In addition, the parameter x gets copied into a register that is not a callee saved, and we are not able to assume that the value has not change in that case, so we do not generate the entry value in that case..

[0] https://llvm.org/docs/SourceLevelDebugging.html#llvm-dbg-value
[1] https://github.com/llvm/llvm-project/blob/master/lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp#L27

[dstenb]

In D68209#1755217, @djtodoro wrote:

At the moment, we should avoid creating the entry value for the sink variable.

At the risk that I'm overlooking something here, but isn't it safe to emit an entry value in that case? The variable is a reference, so the debug location is an address rather than an value. Shouldn't it be safe to emit an entry value for the address regardless of what types of modification are made to the data inside the function? Using the same argument, I assume that there shouldn't be a need to emit a dbg.value after ++sink;?

Here is what GCC emits for that variable:

0x00000050:     DW_TAG_formal_parameter
                  DW_AT_name    ("sink")
                  DW_AT_decl_file       ("/path/to/foo.cpp")
                  DW_AT_decl_line       (12)
                  DW_AT_decl_column     (0x11)
                  DW_AT_type    (0x00000084 "int&")
                  DW_AT_location        (0x0000000c: 
                     [0x0000000000000000, 0x0000000000000006): DW_OP_reg5 RDI
                     [0x0000000000000006, 0x0000000000000012): DW_OP_reg3 RBX
                     [0x0000000000000012, 0x0000000000000013): DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value)

[djtodoro]

Hmm... I am just looking at the GCC's generated case, and it looks like it is OK. I was actually looking at the case where I have changed the parameter to be passed as value...
So yes, it is OK, I have mixed up cases and made a quick comment here, but it was a mistake. Thanks !! :)

But anyhow, some of the cases supported in the lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values_x86_64/TestBasicEntryValuesX86_64.py are now unsupported, so I need to change the test for sure.

[djtodoro]

-Update the LLDB entry-values functionality test

[djtodoro]

Ping.

[jmorse]

This looks ready to go in, although I've no familiarity with the LLDB tests modified in the latest changed. As far as I can see, this is weakening some of the tests (and expecting more unavailable values) because the mechanism for identifying when a value is unchanged is now more conservative, and we drop more entry values?

[djtodoro]

@jmorse Thanks for your comment!

In D68209#1761600, @jmorse wrote:

This looks ready to go in, although I've no familiarity with the LLDB tests modified in the latest changed. As far as I can see, this is weakening some of the tests (and expecting more unavailable values) because the mechanism for identifying when a value is unchanged is now more conservative, and we drop more entry values?

Yes. The modification analysis here is more conservative in some cases, since we can not track a parameter's value copied into a register that is not callee saved (at the moment, unless we find out some improvement for that).
But, current approach will cover some entry values we did not cover so far. For example, in some cases, we can use the entry value of a parameter until it got changed, instead of using the entry values for the parameters which are not changed throughout a whole function.

Since I am not familiar with LLDB so much, I will leave it to @vsk and @aprantl to take another look into the changes.

[vsk]

Thanks for updating lldb. I have no objections to landing this. We can use llvm.org/pr44059 to track strengthening the lldb test again.

[djtodoro]

Thanks for the reviews!

[labath]

Hi,

I'm not sure if you've noticed but the re-enabled TestBasicEntryValuesX86_64.py is failing both on linux and mac. An example of breakage can be found here: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/4338/testReport/ (you can ignore the TestReturnValue.py failure -- that's an unrelated concurrent breakage which has been since fixed).

[djtodoro]

Reverted while investigating. I am not sure what happened, since the test passed on my machine. Thanks!

[vsk]

lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp

159 ↗

(On Diff #231847)

The failure was:

main.cpp:159:21: error: FUNC11-BT-NEXT: expected string not found in input
 // FUNC11-BT-NEXT: func12{{.*}} [artificial]
                    ^
<stdin>:3:2: note: scanning from here
 frame #1: 0x00000001079eae69 a.out`func12(sink=0x00007ffee8215cb4, x=123) at main.cpp:179:3 [opt]

The added DESTROY_RBX asm might confuse TailRecursionElimination into believing that the callee accesses the caller's stack. Could you double-check that a tail call is actually emitted in func12 (something like jmp *%rax)? If it is, this is a pre-existing lldb bug, so the func12 test should be disabled.

[djtodoro]

lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp

159 ↗

(On Diff #231847)

@vsk Thanks for the comment!

The problem here is the fresh change in the code production by using the -O1 level of optimization. More precisely, at very high level, after the D65410 we do not have a tail call where we expected.
I am proposing using the -O2 level of the optimizations, since we are testing printing of the entry values in the test case, rather than tail call frames with particular level of optimization.
WDYT?

[vsk]

lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp

159 ↗

(On Diff #231847)

Sounds good to me, thanks for chasing that down!

llvm/lib/CodeGen/LiveDebugValues.cpp

 static Register isDbgValueDescribedByReg(const MachineInstr &MI) {
   assert(MI.isDebugValue() && "expected a DBG_VALUE");
   assert(MI.getNumOperands() == 4 && "malformed DBG_VALUE");
   // If location of variable is described using a register (directly
   // or indirectly), this register is always a first operand.
   return MI.getOperand(0).isReg() ? MI.getOperand(0).getReg() : Register();
 }
 
+static bool isRegOtherThanSPAndFP(const MachineOperand &Op, MachineInstr &MI,
+                                  const TargetRegisterInfo *TRI) {
+  if (!Op.isReg())
+    return false;
+
+  MachineFunction *MF = MI.getParent()->getParent();
+  const TargetLowering *TLI = MF->getSubtarget().getTargetLowering();
+  unsigned SP = TLI->getStackPointerRegisterToSaveRestore();
+  Register FP = TRI->getFrameRegister(*MF);
+
+  return Op.getReg() != SP && Op.getReg() != FP;
+}
+
 namespace {
 
 class LiveDebugValues : public MachineFunctionPass {
 private:
   const TargetRegisterInfo *TRI;
   const TargetInstrInfo *TII;
   const TargetFrameLowering *TFI;
   BitVector CalleeSavedRegs;
struct VarLoc {
     const MachineInstr &MI;
 
     mutable UserValueScopes UVS;
     enum VarLocKind {
       InvalidKind = 0,
       RegisterKind,
       SpillLocKind,
       ImmediateKind,
-      EntryValueKind
+      EntryValueKind,
+      EntryValueCopyRegKind,
     } Kind = InvalidKind;
 
     /// The value location. Stored separately to avoid repeatedly
     /// extracting it from MI.
     union {
       uint64_t RegNo;
       SpillLoc SpillLocation;
       uint64_t Hash;
       int64_t Immediate;
       const ConstantFP *FPImm;
       const ConstantInt *CImm;
     } Loc;
 
     VarLoc(const MachineInstr &MI, LexicalScopes &LS)
         : Var(MI), Expr(MI.getDebugExpression()), MI(MI),
           UVS(MI.getDebugLoc(), LS) {
       static_assert((sizeof(Loc) == sizeof(uint64_t)),
                     "hash does not cover all members of Loc");
       assert(MI.isDebugValue() && "not a DBG_VALUE");
       assert(MI.getNumOperands() == 4 && "malformed DBG_VALUE");
       if (int RegNo = isDbgValueDescribedByReg(MI)) {
-        Kind = MI.isDebugEntryValue() ? EntryValueKind : RegisterKind;
+        Kind = RegisterKind;
         Loc.RegNo = RegNo;
       } else if (MI.getOperand(0).isImm()) {
         Kind = ImmediateKind;
         Loc.Immediate = MI.getOperand(0).getImm();
       } else if (MI.getOperand(0).isFPImm()) {
         Kind = ImmediateKind;
         Loc.FPImm = MI.getOperand(0).getFPImm();
       } else if (MI.getOperand(0).isCImm()) {
         Kind = ImmediateKind;
         Loc.CImm = MI.getOperand(0).getCImm();
       }
-      assert((Kind != ImmediateKind || !MI.isDebugEntryValue()) &&
-             "entry values must be register locations");
+
+      // We create the debug entry values from the factory functions rather than
+      // from this ctor.
+      assert(Kind != EntryValueKind && Kind != EntryValueCopyRegKind);
     }
 
     /// Take the variable and machine-location in DBG_VALUE MI, and build an
     /// entry location using the given expression.
     static VarLoc CreateEntryLoc(const MachineInstr &MI, LexicalScopes &LS,
                                  const DIExpression *EntryExpr) {
       VarLoc VL(MI, LS);
       VL.Kind = EntryValueKind;
       VL.Expr = EntryExpr;
       return VL;
     }
 
+    /// Take the variable and the entry value expression from an entry DBG_VALUE,
+    /// and make a copy location of the debug entry value by setting the
+    /// register to the NewReg.
+    static VarLoc CreateCopyEntryLoc(const MachineInstr &MI, LexicalScopes &LS,
+                                     const DIExpression *EntryExpr,
+                                     unsigned NewReg) {
+      VarLoc VL(MI, LS);
+      VL.Kind = EntryValueCopyRegKind;
+      VL.Expr = EntryExpr;
+      VL.Loc.RegNo = NewReg;
+      return VL;
+    }
+
     /// Copy the register location in DBG_VALUE MI, updating the register to
     /// be NewReg.
     static VarLoc CreateCopyLoc(const MachineInstr &MI, LexicalScopes &LS,
                                 unsigned NewReg) {
       VarLoc VL(MI, LS);
       assert(VL.Kind == RegisterKind);
       VL.Loc.RegNo = NewReg;
       return VL;
MachineInstr *BuildDbgValue(MachineFunction &MF) const {
       const DebugLoc &DbgLoc = MI.getDebugLoc();
       bool Indirect = MI.isIndirectDebugValue();
       const auto &IID = MI.getDesc();
       const DILocalVariable *Var = MI.getDebugVariable();
       const DIExpression *DIExpr = MI.getDebugExpression();
 
       switch (Kind) {
       case EntryValueKind:
+      case EntryValueCopyRegKind:
         // An entry value is a register location -- but with an updated
-        // expression.
-        return BuildMI(MF, DbgLoc, IID, Indirect, Loc.RegNo, Var, Expr);
+        // expression. The register location of such DBG_VALUE is always the one
+        // from the entry DBG_VALUE, it does not matter if the entry value was
+        // copied in to another register due to some optimizations.
+        return BuildMI(MF, DbgLoc, IID, Indirect, MI.getOperand(0).getReg(),
+                       Var, Expr);
       case RegisterKind:
         // Register locations are like the source DBG_VALUE, but with the
         // register number from this VarLoc.
         return BuildMI(MF, DbgLoc, IID, Indirect, Loc.RegNo, Var, DIExpr);
       case SpillLocKind: {
         // Spills are indirect DBG_VALUEs, with a base register and offset.
         // Use the original DBG_VALUEs expression to build the spilt location
         // on top of. FIXME: spill locations created before this pass runs
struct VarLoc {
     }
 
     /// Is the Loc field a constant or constant object?
     bool isConstant() const { return Kind == ImmediateKind; }
 
     /// If this variable is described by a register, return it,
     /// otherwise return 0.
     unsigned isDescribedByReg() const {
-      if (Kind == RegisterKind)
+      if (Kind == RegisterKind || Kind == EntryValueCopyRegKind)
         return Loc.RegNo;
       return 0;
     }
 
     /// Determine whether the lexical scope of this value's debug location
     /// dominates MBB.
     bool dominates(MachineBasicBlock &MBB) const { return UVS.dominates(&MBB); }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
     // TRI can be null.
     void dump(const TargetRegisterInfo *TRI, raw_ostream &Out = dbgs()) const {
       dbgs() << "VarLoc(";
       switch (Kind) {
       case RegisterKind:
       case EntryValueKind:
+      case EntryValueCopyRegKind:
         dbgs() << printReg(Loc.RegNo, TRI);
         break;
       case SpillLocKind:
         dbgs() << printReg(Loc.SpillLocation.SpillBase, TRI);
         dbgs() << "[" << Loc.SpillLocation.SpillOffset << "]";
         break;
       case ImmediateKind:
         dbgs() << Loc.Immediate;
#endif
 
     /// This operator guarantees that VarLocs are sorted by Variable first.
     bool operator<(const VarLoc &Other) const {
       return std::tie(Var, Kind, Loc.Hash, Expr) <
              std::tie(Other.Var, Other.Kind, Other.Loc.Hash, Other.Expr);
     }
   };
 
-  using DebugParamMap = SmallDenseMap<const DILocalVariable *, MachineInstr *>;
+  // Types for recording a parameter entry value. For a given parameter we
+  // record its debug entry value and potential movement of such value.
+  struct EntryValue {
+    // The debug entry value.
+    MachineInstr *DbgEntryValue;
+
+    // A copy like instruction that moves the entry value around.
+    MachineInstr *DbgEntryValueMovement;
+
+    void setDbgEntryValueMovement(MachineInstr *MI) {
+      DbgEntryValueMovement = MI;
+    }
+  };
+  using DebugParamMap = SmallDenseMap<const DILocalVariable *, EntryValue>;
+
   using VarLocMap = UniqueVector<VarLoc>;
   using VarLocSet = SparseBitVector<>;
   using VarLocInMBB = SmallDenseMap<const MachineBasicBlock *, VarLocSet>;
   struct TransferDebugPair {
     MachineInstr *TransferInst; /// Instruction where this transfer occurs.
     unsigned LocationID;        /// Location number for the transfer dest.
   };
   using TransferMap = SmallVector<TransferDebugPair, 4>;
public:
     /// Empty the set.
     void clear() {
       VarLocs.clear();
       Vars.clear();
     }
 
     /// Return whether the set is empty or not.
     bool empty() const {
       assert(Vars.empty() == VarLocs.empty() && "open ranges are inconsistent");
       return VarLocs.empty();
     }
   };
 
   /// Tests whether this instruction is a spill to a stack location.
   bool isSpillInstruction(const MachineInstr &MI, MachineFunction *MF);
 
   /// Decide if @MI is a spill instruction and return true if it is. We use 2
   /// criteria to make this decision:
#endif
   /// address the spill location in a target independent way.
   VarLoc::SpillLoc extractSpillBaseRegAndOffset(const MachineInstr &MI);
   void insertTransferDebugPair(MachineInstr &MI, OpenRangesSet &OpenRanges,
                                TransferMap &Transfers, VarLocMap &VarLocIDs,
                                unsigned OldVarID, TransferKind Kind,
                                unsigned NewReg = 0);
 
   void transferDebugValue(const MachineInstr &MI, OpenRangesSet &OpenRanges,
-                          VarLocMap &VarLocIDs);
+                          VarLocMap &VarLocIDs, DebugParamMap &DebugEntryVals);
   void transferSpillOrRestoreInst(MachineInstr &MI, OpenRangesSet &OpenRanges,
                                   VarLocMap &VarLocIDs, TransferMap &Transfers);
+  void removeOrSalvageEntryValue(const MachineInstr &MI,
+                                 const DILocalVariable *Var,
+                                 const DIExpression *Expr,
+                                 DebugParamMap &DebugEntryVals);
   void emitEntryValues(MachineInstr &MI, OpenRangesSet &OpenRanges,
                        VarLocMap &VarLocIDs, TransferMap &Transfers,
                        DebugParamMap &DebugEntryVals,
                        SparseBitVector<> &KillSet);
   void transferRegisterCopy(MachineInstr &MI, OpenRangesSet &OpenRanges,
-                            VarLocMap &VarLocIDs, TransferMap &Transfers);
+                            VarLocMap &VarLocIDs, TransferMap &Transfers,
+                            DebugParamMap &DebugEntryVals);
   void transferRegisterDef(MachineInstr &MI, OpenRangesSet &OpenRanges,
                            VarLocMap &VarLocIDs, TransferMap &Transfers,
                            DebugParamMap &DebugEntryVals);
   bool transferTerminator(MachineBasicBlock *MBB, OpenRangesSet &OpenRanges,
                           VarLocInMBB &OutLocs, const VarLocMap &VarLocIDs);
 
   void process(MachineInstr &MI, OpenRangesSet &OpenRanges,
                VarLocInMBB &OutLocs, VarLocMap &VarLocIDs,
                TransferMap &Transfers, DebugParamMap &DebugEntryVals,
-               OverlapMap &OverlapFragments,
-               VarToFragments &SeenFragments);
+               OverlapMap &OverlapFragments, VarToFragments &SeenFragments);
 
   void accumulateFragmentMap(MachineInstr &MI, VarToFragments &SeenFragments,
                              OverlapMap &OLapMap);
 
   bool join(MachineBasicBlock &MBB, VarLocInMBB &OutLocs, VarLocInMBB &InLocs,
             const VarLocMap &VarLocIDs,
             SmallPtrSet<const MachineBasicBlock *, 16> &Visited,
             SmallPtrSetImpl<const MachineBasicBlock *> &ArtificialBlocks,
assert(PVal->kind() == PseudoSourceValue::FixedStack &&
          "Inconsistent memory operand in spill instruction");
   int FI = cast<FixedStackPseudoSourceValue>(PVal)->getFrameIndex();
   const MachineBasicBlock *MBB = MI.getParent();
   unsigned Reg;
   int Offset = TFI->getFrameIndexReference(*MBB->getParent(), FI, Reg);
   return {Reg, Offset};
 }
 
+/// Try to salvage the debug entry value if encounter a new debug value
+/// describing the same parameter or stop tracking it.
+void LiveDebugValues::removeOrSalvageEntryValue(const MachineInstr &MI,
+                                                const DILocalVariable *Var,
+                                                const DIExpression *Expr,
+                                                DebugParamMap &DebugEntryVals) {
+  const MachineInstr *DebugEntryValue = DebugEntryVals[Var].DbgEntryValue;
+  assert(DebugEntryValue->getOperand(0).isReg() &&
+         "Entry value must be register location");
+
+  // Skip the DBG_VALUE which is the debug entry value itself.
+  if (MI.isIdenticalTo(*DebugEntryValue))
+    return;
+
+  // Check if the DBG_VALUE describes the register holding a copy of the debug
+  // entry value.
+  if (DebugEntryVals[Var].DbgEntryValueMovement) {
+    Register EntryValueCopyReg =
+        DebugEntryVals[Var].DbgEntryValueMovement->getOperand(0).getReg();
+    if (MI.getOperand(0).isReg() &&
+        MI.getOperand(0).getReg() == EntryValueCopyReg) {
+      if (MI.getDebugExpression()->getNumElements() == 0)
+        return;
+    }
+  }
+
+  // Remove the debug entry value from the map if we encounter a different
+  // DBG_VALUE describing the same parameter, because that can imply that the
+  // parameter's value has changed, so we can not rely on its entry value any
+  // more.
+  Register EntryValueReg = DebugEntryValue->getOperand(0).getReg();
+  if (MI.getOperand(0).isReg() && MI.getOperand(0).getReg() != EntryValueReg) {
+    LLVM_DEBUG(dbgs() << "Deleting a DBG entry value because of: ";
+               MI.print(dbgs(), /*IsStandalone*/ false,
+                        /*SkipOpers*/ false, /*SkipDebugLoc*/ false,
+                        /*AddNewLine*/ true, TII));
+    DebugEntryVals.erase(Var);
+    return;
+  }
+}
+
 /// End all previous ranges related to @MI and start a new range from @MI
 /// if it is a DBG_VALUE instr.
-void LiveDebugValues::transferDebugValue(const MachineInstr &MI,
-                                         OpenRangesSet &OpenRanges,
-                                         VarLocMap &VarLocIDs) {
+void LiveDebugValues::transferDebugValue(
+    const MachineInstr &MI, OpenRangesSet &OpenRanges, VarLocMap &VarLocIDs,
+    DebugParamMap &DebugEntryVals) {
   if (!MI.isDebugValue())
     return;
   const DILocalVariable *Var = MI.getDebugVariable();
   const DIExpression *Expr = MI.getDebugExpression();
   const DILocation *DebugLoc = MI.getDebugLoc();
   const DILocation *InlinedAt = DebugLoc->getInlinedAt();
   assert(Var->isValidLocationForIntrinsic(DebugLoc) &&
          "Expected inlined-at fields to agree");
 
+  // Check if we should stop tracking the debug entry value.
+  if (Var->isParameter() && DebugEntryVals.count(Var))
+    removeOrSalvageEntryValue(MI, Var, Expr, DebugEntryVals);
+
   // End all previous ranges of Var.
   DebugVariable V(Var, Expr, InlinedAt);
   OpenRanges.erase(V);
 
   // Add the VarLoc to OpenRanges from this DBG_VALUE.
   unsigned ID;
   if (isDbgValueDescribedByReg(MI) || MI.getOperand(0).isImm() ||
       MI.getOperand(0).isFPImm() || MI.getOperand(0).isCImm()) {
void LiveDebugValues::emitEntryValues(MachineInstr &MI,
   for (unsigned ID : KillSet) {
     if (!VarLocIDs[ID].Var.getVar()->isParameter())
       continue;
 
     const MachineInstr *CurrDebugInstr = &VarLocIDs[ID].MI;
 
     // If parameter's DBG_VALUE is not in the map that means we can't
     // generate parameter's entry value.
-    if (!DebugEntryVals.count(CurrDebugInstr->getDebugVariable()))
+    auto ParamEntryValue =
+        DebugEntryVals.find(CurrDebugInstr->getDebugVariable());
+    if (ParamEntryValue == DebugEntryVals.end())
       continue;
 
-    auto ParamDebugInstr = DebugEntryVals[CurrDebugInstr->getDebugVariable()];
+    auto ParamDebugInstr = ParamEntryValue->second.DbgEntryValue;
+    auto ParamEntryValMovement = ParamEntryValue->second.DbgEntryValueMovement;
     DIExpression *NewExpr = DIExpression::prepend(
         ParamDebugInstr->getDebugExpression(), DIExpression::EntryValue);
 
-    VarLoc EntryLoc = VarLoc::CreateEntryLoc(*ParamDebugInstr, LS, NewExpr);
+    auto getEntryLoc = [ParamDebugInstr, ParamEntryValMovement,
+                        NewExpr](LexicalScopes &LS) {
+      if (ParamEntryValMovement)
+        return VarLoc::CreateCopyEntryLoc(
+            *ParamDebugInstr, LS, NewExpr,
+            ParamEntryValMovement->getOperand(0).getReg());
+      return VarLoc::CreateEntryLoc(*ParamDebugInstr, LS, NewExpr);
+    };
+
+    VarLoc EntryLoc = getEntryLoc(LS);
 
     unsigned EntryValLocID = VarLocIDs.insert(EntryLoc);
     Transfers.push_back({&MI, EntryValLocID});
     OpenRanges.insert(EntryValLocID, EntryLoc.Var);
   }
 }
 
 /// Create new TransferDebugPair and insert it in \p Transfers. The VarLoc
LLVM_DEBUG({
       dbgs() << "Creating VarLoc for spill:";
       VL.dump(TRI);
     });
     return;
   }
   case TransferKind::TransferRestore: {
     assert(NewReg &&
            "No register supplied when handling a restore of a debug value");
-    MachineFunction *MF = MI.getMF();
-    DIBuilder DIB(*const_cast<Function &>(MF->getFunction()).getParent());
     // DebugInstr refers to the pre-spill location, therefore we can reuse
     // its expression.
     VarLoc VL = VarLoc::CreateCopyLoc(*DebugInstr, LS, NewReg);
     ProcessVarLoc(VL);
     LLVM_DEBUG({
       dbgs() << "Creating VarLoc for restore:";
       VL.dump(TRI);
     });

 }
 
 /// If \p MI is a register copy instruction, that copies a previously tracked
 /// value from one register to another register that is callee saved, we
 /// create new DBG_VALUE instruction  described with copy destination register.
 void LiveDebugValues::transferRegisterCopy(MachineInstr &MI,
                                            OpenRangesSet &OpenRanges,
                                            VarLocMap &VarLocIDs,
-                                           TransferMap &Transfers) {
+                                           TransferMap &Transfers,
+                                           DebugParamMap &DebugEntryVals) {
   const MachineOperand *SrcRegOp, *DestRegOp;
 
-  if (!TII->isCopyInstr(MI, SrcRegOp, DestRegOp) || !SrcRegOp->isKill() ||
-      !DestRegOp->isDef())
+  if (!TII->isCopyInstr(MI, SrcRegOp, DestRegOp))
+    return;
+
+  if (!DestRegOp->isDef())
     return;
 
   auto isCalleSavedReg = [&](unsigned Reg) {
     for (MCRegAliasIterator RAI(Reg, TRI, true); RAI.isValid(); ++RAI)
       if (CalleeSavedRegs.test(*RAI))
         return true;
     return false;
   };
 
   Register SrcReg = SrcRegOp->getReg();
   Register DestReg = DestRegOp->getReg();
 
   // We want to recognize instructions where destination register is callee
   // saved register. If register that could be clobbered by the call is
   // included, there would be a great chance that it is going to be clobbered
   // soon. It is more likely that previous register location, which is callee
   // saved, is going to stay unclobbered longer, even if it is killed.
   if (!isCalleSavedReg(DestReg))
     return;
 
+  auto isCopyOfTheEntryValue = [SrcReg](const EntryValue &ParamEntryValue) {
+    // Check if it is a copy of the entry value register.
+    if (ParamEntryValue.DbgEntryValue->getOperand(0).getReg() == SrcReg)
+      return true;
+
+    // Check if it is a copy of the register holding a copy of the entry value.
+    if (ParamEntryValue.DbgEntryValueMovement &&
+        ParamEntryValue.DbgEntryValueMovement->getOperand(0).getReg() == SrcReg)
+      return true;
+
+    return false;
+  };
+
+  // Remember an entry value movement. If we encounter a new debug value of
+  // a parameter describing only a moving of the value around, rather then
+  // modifying it, we are still able to use the entry value if needed.
+  if (isRegOtherThanSPAndFP(*DestRegOp, MI, TRI)) {
+    for (auto &EnltyValue : DebugEntryVals) {
+      if (isCopyOfTheEntryValue(EnltyValue.second)) {
+        EnltyValue.second.setDbgEntryValueMovement(&MI);
+        break;
+      }
+    }
+  }
+
+  if (!SrcRegOp->isKill())
+    return;
+
   for (unsigned ID : OpenRanges.getVarLocs()) {
     if (VarLocIDs[ID].isDescribedByReg() == SrcReg) {
       insertTransferDebugPair(MI, OpenRanges, Transfers, VarLocIDs, ID,
                               TransferKind::TransferCopy, DestReg);
       return;
     }
   }
 }

 
 /// This routine creates OpenRanges and OutLocs.
 void LiveDebugValues::process(MachineInstr &MI, OpenRangesSet &OpenRanges,
                               VarLocInMBB &OutLocs, VarLocMap &VarLocIDs,
                               TransferMap &Transfers,
                               DebugParamMap &DebugEntryVals,
                               OverlapMap &OverlapFragments,
                               VarToFragments &SeenFragments) {
-  transferDebugValue(MI, OpenRanges, VarLocIDs);
-  transferRegisterDef(MI, OpenRanges, VarLocIDs, Transfers,
-                      DebugEntryVals);
-  transferRegisterCopy(MI, OpenRanges, VarLocIDs, Transfers);
+  transferDebugValue(MI, OpenRanges, VarLocIDs, DebugEntryVals);
+  transferRegisterDef(MI, OpenRanges, VarLocIDs, Transfers, DebugEntryVals);
+  transferRegisterCopy(MI, OpenRanges, VarLocIDs, Transfers, DebugEntryVals);
   transferSpillOrRestoreInst(MI, OpenRanges, VarLocIDs, Transfers);
 }
 
 /// This routine joins the analysis results of all incoming edges in @MBB by
 /// inserting a new DBG_VALUE instruction at the start of the @MBB - if the same
 /// source variable in all the predecessors of @MBB reside in the same location.
 bool LiveDebugValues::join(
     MachineBasicBlock &MBB, VarLocInMBB &OutLocs, VarLocInMBB &InLocs,
bool LiveDebugValues::ExtendRanges(MachineFunction &MF) {
   DenseMap<MachineBasicBlock *, unsigned int> BBToOrder;
   std::priority_queue<unsigned int, std::vector<unsigned int>,
                       std::greater<unsigned int>>
       Worklist;
   std::priority_queue<unsigned int, std::vector<unsigned int>,
                       std::greater<unsigned int>>
       Pending;
 
-  const TargetLowering *TLI = MF.getSubtarget().getTargetLowering();
-  unsigned SP = TLI->getStackPointerRegisterToSaveRestore();
-  Register FP = TRI->getFrameRegister(MF);
-  auto IsRegOtherThanSPAndFP = [&](const MachineOperand &Op) -> bool {
-    return Op.isReg() && Op.getReg() != SP && Op.getReg() != FP;
-  };
-
   // Working set of currently collected debug variables mapped to DBG_VALUEs
   // representing candidates for production of debug entry values.
   DebugParamMap DebugEntryVals;
 
   MachineBasicBlock &First_MBB = *(MF.begin());
   // Only in the case of entry MBB collect DBG_VALUEs representing
   // function parameters in order to generate debug entry values for them.
   // Currently, we generate debug entry values only for parameters that are
   // unmodified throughout the function and located in a register.
   // TODO: Add support for parameters with non-empty debug expressions (for
   // example fragments).
   // TODO: Add support for modified arguments that can be expressed
   // by using its entry value.
   // TODO: Add support for local variables that are expressed in terms of
   // parameters entry values.
   for (auto &MI : First_MBB)
     if (MI.isDebugValue() && !MI.getDebugLoc()->getInlinedAt() &&
-        !MI.isIndirectDebugValue() && IsRegOtherThanSPAndFP(MI.getOperand(0)) &&
+        MI.isDebugValue() && MI.getDebugVariable()->isParameter() &&
+        !MI.isIndirectDebugValue() &&
+        isRegOtherThanSPAndFP(MI.getOperand(0), MI, TRI) &&
         !DebugEntryVals.count(MI.getDebugVariable()) &&
         MI.getDebugExpression()->getNumElements() == 0)
-      DebugEntryVals[MI.getDebugVariable()] = &MI;
+      DebugEntryVals[MI.getDebugVariable()] = EntryValue{&MI, nullptr};
 
   // Initialize per-block structures and scan for fragment overlaps.
   for (auto &MBB : MF) {
     PendingInLocs[&MBB] = VarLocSet();
 
     for (auto &MI : MBB) {
       if (MI.isDebugValue())
         accumulateFragmentMap(MI, SeenFragments, OverlapFragments);

llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir

+# RUN: llc -debug-entry-values -run-pass=livedebugvalues -march=x86-64 -o - %s | FileCheck %s
+#
+#extern void fn1 (int, int, int);
+#
+#__attribute__((noinline))
+#int
+#fn2 (int a, int b, int c) {
+#  int q = 2 + a;
+#
+#  fn1 (5, 6, q);
+#
+#  b = b + 7;
+#  if (b < 17)
+#    return 1;
+#  else
+#    return 0;
+#}
+#
+# CHECK: DBG_VALUE $edi, $noreg, !16, !DIExpression(DW_OP_LLVM_entry_value, 1), debug-location {{.*}}
+# CHECK: DBG_VALUE $edx, $noreg, !18, !DIExpression(DW_OP_LLVM_entry_value, 1), debug-location {{.*}}
+# CHECK: DBG_VALUE $edi, $noreg, !16, !DIExpression(DW_OP_LLVM_entry_value, 1), debug-location {{.*}}
+# CHECK-NOT: DBG_VALUE $esi, $noreg, !17, !DIExpression(DW_OP_LLVM_entry_value, 1, DW_OP_plus_uconst, 7, DW_OP_stack_value), debug-location {{.*}}
+#
+--- |
+  ; ModuleID = 'test.c'
+  source_filename = "test.c"
+  target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+  
+  ; Function Attrs: noinline nounwind uwtable
+  define dso_local i32 @fn2(i32 %a, i32 %b, i32 %c) local_unnamed_addr !dbg !12 {
+  entry:
+    call void @llvm.dbg.value(metadata i32 %a, metadata !16, metadata !DIExpression()), !dbg !20
+    call void @llvm.dbg.value(metadata i32 %b, metadata !17, metadata !DIExpression()), !dbg !20
+    call void @llvm.dbg.value(metadata i32 %c, metadata !18, metadata !DIExpression()), !dbg !20
+    %add = add nsw i32 %a, 2, !dbg !21
+    call void @llvm.dbg.value(metadata i32 %add, metadata !19, metadata !DIExpression()), !dbg !20
+    tail call void @fn1(i32 5, i32 6, i32 %add) #3, !dbg !22
+    call void @llvm.dbg.value(metadata i32 %b, metadata !17, metadata !DIExpression(DW_OP_plus_uconst, 7, DW_OP_stack_value)), !dbg !20
+    %cmp = icmp slt i32 %b, 10, !dbg !23
+    %. = zext i1 %cmp to i32, !dbg !25
+    ret i32 %., !dbg !26
+  }
+  
+  declare !dbg !4 dso_local void @fn1(i32, i32, i32) local_unnamed_addr
+  
+  ; Function Attrs: nounwind readnone speculatable willreturn
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!8, !9, !10}
+  !llvm.ident = !{!11}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 10.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, nameTableKind: None)
+  !1 = !DIFile(filename: "test.c", directory: "/dir")
+  !2 = !{}
+  !3 = !{!4}
+  !4 = !DISubprogram(name: "fn1", scope: !1, file: !1, line: 1, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+  !5 = !DISubroutineType(types: !6)
+  !6 = !{null, !7, !7, !7}
+  !7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !8 = !{i32 2, !"Dwarf Version", i32 4}
+  !9 = !{i32 2, !"Debug Info Version", i32 3}
+  !10 = !{i32 1, !"wchar_size", i32 4}
+  !11 = !{!"clang version 10.0.0"}
+  !12 = distinct !DISubprogram(name: "fn2", scope: !1, file: !1, line: 5, type: !13, scopeLine: 6, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !15)
+  !13 = !DISubroutineType(types: !14)
+  !14 = !{!7, !7, !7, !7}
+  !15 = !{!16, !17, !18, !19}
+  !16 = !DILocalVariable(name: "a", arg: 1, scope: !12, file: !1, line: 5, type: !7)
+  !17 = !DILocalVariable(name: "b", arg: 2, scope: !12, file: !1, line: 5, type: !7)
+  !18 = !DILocalVariable(name: "c", arg: 3, scope: !12, file: !1, line: 5, type: !7)
+  !19 = !DILocalVariable(name: "q", scope: !12, file: !1, line: 7, type: !7)
+  !20 = !DILocation(line: 0, scope: !12)
+  !21 = !DILocation(line: 7, column: 15, scope: !12)
+  !22 = !DILocation(line: 9, column: 5, scope: !12)
+  !23 = !DILocation(line: 12, column: 11, scope: !24)
+  !24 = distinct !DILexicalBlock(scope: !12, file: !1, line: 12, column: 9)
+  !25 = !DILocation(line: 0, scope: !24)
+  !26 = !DILocation(line: 16, column: 1, scope: !12)
+
+...
+---
+name:            fn2
+alignment:       16
+callSites:
+  - { bb: 0, offset: 14, fwdArgRegs: 
+      - { arg: 0, reg: '$edi' }
+      - { arg: 1, reg: '$esi' }
+      - { arg: 2, reg: '$edx' } }
+body:             |
+  bb.0.entry:
+    liveins: $edi, $esi, $rbx
+  
+    DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !20
+    DBG_VALUE $esi, $noreg, !17, !DIExpression(), debug-location !20
+    DBG_VALUE $edx, $noreg, !18, !DIExpression(), debug-location !20
+    frame-setup PUSH64r killed $rbx, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 16
+    CFI_INSTRUCTION offset $rbx, -16
+    $ebx = MOV32rr $esi
+    DBG_VALUE $ebx, $noreg, !17, !DIExpression(), debug-location !20
+    renamable $edi = KILL $edi, implicit-def $rdi
+    DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !20
+    renamable $edx = LEA64_32r killed renamable $rdi, 1, $noreg, 2, $noreg, debug-location !21
+    DBG_VALUE $edx, $noreg, !19, !DIExpression(), debug-location !20
+    $edi = MOV32ri 5, debug-location !22
+    $esi = MOV32ri 6, debug-location !22
+    CALL64pcrel32 @fn1, csr_64, implicit $rsp, implicit $ssp, implicit $edi, implicit $esi, implicit $edx, implicit-def $rsp, implicit-def $ssp, debug-location !22
+    DBG_VALUE $ebx, $noreg, !17, !DIExpression(DW_OP_plus_uconst, 7, DW_OP_stack_value), debug-location !20
+    renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, debug-location !23
+    CMP32ri8 killed renamable $ebx, 10, implicit-def $eflags, debug-location !23
+    renamable $al = SETCCr 12, implicit killed $eflags, implicit killed $eax, implicit-def $eax, debug-location !23
+    $rbx = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !26
+    CFI_INSTRUCTION def_cfa_offset 8, debug-location !26
+    RETQ $eax, debug-location !26
+
+...