In D133310#3792255, @Orlando wrote:

Thanks for taking a look

Just a couple of typos.

A typo and a couple of nitish suggestions.

In D129636#3716487, @jmorse wrote:

Did you consider that forgetValue in fact does not only forget the value you are aiming, but also all its users recursively? Not SCEV users (they likely don't exist), but instruction users.

Ah, that rather sounds like a blocker -- if there's no way to limit what's forgotten to "just the things unique to this Value", it's going to be impossible to peel apart "what was important to debug-info" and "what was important to codegen". A quick look at the SCEV methods suggests that plumbing in an argument flag is going to be pretty invasive, ScalarEvolution::getSCEV fans out to a lot of functions with a lot of logic.

Stephen wrote:

Though I'm also surprised that just adding something to the cache is modifying the output

I imagine you might find a different way expanding expressions for Values, depending on which PHIs you look at first, which changes if you look at the dbg.values first.

The best alternative I can suggest is to create a new ScalarEvolution instance and use it for dbg value queries. It should be good enough if the only purpose of that is to find undefs.

I'm in two minds about this -- it solves all the problems raised (obviously), on the other hand I imagine it doubles the amount of work done, which isn't ideal. However, I think the reason why we're using ScalarEvolution in this way is because all the PHIs might get replaced during loop-strength-reduction, and we need to use ScalarEvolution's map of old-to-new PHIs to reconstruct the dbg.values operands -- is that right @chrisjackson ?

In D129636#3715477, @markus wrote:

In D129636#3715417, @mkazantsev wrote:

The best alternative I can suggest is to create a new ScalarEvolution instance and use it for dbg value queries. It should be good enough if the only purpose of that is to find undefs.

That does sound like a very tempting alternative to be sure that these sort of issues do not crop up again. Not sure if it is possible though (@chrisjackson?)

In D129636#3715417, @mkazantsev wrote:

Did you consider that forgetValue in fact does not only forget the value you are aiming, but also all its users recursively? Not SCEV users (they likely don't exist), but instruction users.

Imagine that some of the users had no-wrap flags inferred from other queries. Now you forget it, and the flags get lost. Won't this introruce a similar issue in another place? It may also affect some loops into which these values are involved.

Generally, exposing API about existing SCEVs isn't a great idea, and I don't see enough justification in your case.

The best alternative I can suggest is to create a new ScalarEvolution instance and use it for dbg value queries. It should be good enough if the only purpose of that is to find undefs.

Anyways, why do you need SCEV at all to find undefs? Can we instead traverse through instructions and get rid of SCEV query at all?

I think this is a succinct solution and I don't perceive a problem with exposing the extra SCEV method, as I've done something similar myself in the past, as mentioned above. I would like to see a comment explaining the need to call forget(), otherwise lgtm.

In D129636#3699269, @fhahn wrote:

In D129636#3698967, @chrisjackson wrote:

In D129636#3696750, @fhahn wrote:

I think I am missing some context here, but this looks a bit suspicious. I am not sure if exposing and checking whether a SCEV exists is the right way to go.

IIUC the main issue seems to be that we try to create SCEVs for all dbg intrinsics? Would it be possible to instead only get SCEV expressions for values we really need (that/s the induction variable I assume, for which there already should be SCEV expression)?

It is necessary to obtain SCEVs for all the values used in dbg.value within the loop, as LSR may optimise out these values and these are salvaged by generating a DWARF expression from the SCEV expression. For further explanation see:
D105207 and the EuroLLVM2022 talk on the method here :SCEV-Based debuginfo salvaging in Loop Strength Reduction.

Right, but wouldn't it be sufficient to collect all expressions LSR optimizes? For those, it will already have created SCEV expressions for. This might be more difficult, but I am wondering if it would be feasible at least in theory.

In D129636#3696750, @fhahn wrote:

I think I am missing some context here, but this looks a bit suspicious. I am not sure if exposing and checking whether a SCEV exists is the right way to go.

IIUC the main issue seems to be that we try to create SCEVs for all dbg intrinsics? Would it be possible to instead only get SCEV expressions for values we really need (that/s the induction variable I assume, for which there already should be SCEV expression)?

In D129636#3696435, @markus wrote:

Not sure if this ended up so well as it appears that getExistingSCEV was a private method and I am uncertain if we can or should make it public. Either way if we decide to go this route then we probably should add explicit tests for this part as well (i.e. to make sure that the forgetting of SCEV does not produce another invariance issue).

In D129636#3696260, @markus wrote:

In D129636#3696240, @chrisjackson wrote:

I'm looking at the forgetValue() implementation. I think that as long as the SCEV for the value is preserved and accessable in SalvageDVI() then all is well in terms of the salvaging method.

Right, I think that forgetting a SCEV does not invalidate the SCEV itself it is just that if we were to ask for it again then we would get a new object (that would still be equivalent though not the same object pointer wise).

In D129636#3648003, @jmorse wrote:

Thanks for hunting this down, caching layers affecting codegen sounds hard to debug.

I'm no SCEV expert, so a couple of possibly obvious questions:

• Is it safe to store the SCEV* for a Value that's then forgotten?
• Is there a risk that the presence of debug-info will now cause a SCEV to be forgotten that _should_ have been remembered, because of unconditionally forgetting a SCEV if it's used by debug-info?

I think the second point is the crux here. Perhaps there is a way to test if the SCEV already exists, then call forget conditionally.

A few comments up I wrote which I think does what you are requesting

For the second point maybe ScalarEvolution::getExistingSCEV() could be used to see if this SCEV existed before analyzing the dbg.value or not so that we don't end up forgetting about values that we would not forget about hadn't it been for the dbg.value.

If that makes sense I will update the patch.

I'm looking at the forgetValue() implementation. I think that as long as the SCEV for the value is preserved and accessable in SalvageDVI() then all is well in terms of the salvaging method.

In D129636#3654504, @markus wrote:

I'd still like to get some feedback from the author of the LSR debug salvaging (@chrisjackson) before proceeding as at the moment it is a bit unclear if it actually is safe to do a SE.forgetValue() here.

Address reviewer comments by adding a clarification in the llvm-ar tool-name test.

• Add managed DVIRecoveryRecords to address memory leak

Respond to reviewer comments:

• Add missed llvm-ranlib/tool-name.test update
• Make ranlib printout consistent with ar
• Make toolname pattern check more strict

Note that this patch also fixes Issue #55097.

Made corrections to UpdateDbgValueInst() using @StephenTozer 's help.

Removed old comments, clarified messages in some asserts. Added some comments to methods.

Correct diff update file.

Attempt to correct the final overwrite of the dbg.value, based on the count of DW_OP_llvm_arg operands in the expression.

Respond to the very helpful review from @StephenTozer. Includes:

• SCEVDbgValueBuilder.appendToVectors() now merges location vectors, preventing duplicates
• Improved check for when a shorter DIExpression can be emitted
• Use isComplex() instead of the size of the expression vector

Update some variable names and a couple of comments to improve readability.

Change remaining Densemap (for updated location-op indexes) to a SmallVector.

Apply @djtodoro 's change and clang-format.

Mostly refactoring of the functions that write the final salvage dbg.value. Some small corrections after a Clang-3.4 build caught a couple of corner cases (it builds fine now).

A WIP update only, not ready for a re-review. Incorporated some of @StephenTozer 's suggestions e.g.

Restored diff. I accidentally updated this diff with a file intended for a separate issue.

-EDIT-
Diff changed in error.

Add check for empty arglist that was missing from check for nullptr arglist member

Restore the the old behaviour for transforming DIArglist location arguments to single- location op:. When trying to determine the type of an undef Value that has become a nullptr, simply abandon the salvage attempt. This restores the NFC status of this patch.

In D120168#3367442, @Orlando wrote:

In D120168#3363746, @chrisjackson wrote:

Reinstate the check for dbg.value that LSR has changed from single location-op to multi location-op. When LSR makes this change, the single location argument is changed to a DIArglist. This patch now replaces the DIArglist with a single 'undef'. This will prevent the generation of invalid dbg.value that were causing the crash as reported by @Orlando .

If that change makes the patch not-NFC, please could you add a test for this and mention it in the commit message? Lastly, please can you appease the clang-format bot?

Thanks @StephenTozer , these are really helpful. I will be able to address them once patch [1/2] is done, which shouldn't be much longer, so hopefully I can address your comments at the end of the week.

Remove accidental double application of dbg.value expression restoration.

Reinstate the check for dbg.value that LSR has changed from single location-op to multi location-op. When LSR makes this change, the single location argument is changed to a DIArglist. This patch now replaces the DIArglist with a single 'undef'. This will prevent the generation of invalid dbg.value that were causing the crash as reported by @Orlando .

In D120168#3363344, @Orlando wrote:

The change SGTM but I found a reproducer that causes clang to crash when this patch is applied:

$ cat reduce.cpp
#include <utility>
template <typename a> class Vec {
public:
  void push(a);
};
void d(int argc, char **argv) {
  Vec<std::pair<char *, unsigned>> e;
  for (int f = 0;;)
    for (; f < argc; ++f)
      e.push(std::make_pair(argv[f], f));
}

$ clang++  -O2 -g reduce.cpp

/home/och/dev/llvm/llvm-project/llvm/lib/CodeGen/AsmPrinter/DebugLocEntry.h:139: llvm::DbgValueLoc::DbgValueLoc(const llvm::DIExpression*, llvm::ArrayRef<llvm::DbgValueLocEntry>, bool): Assertion `Expr && Expr->isValid() && is_contained(Expr->getElements(), dwarf::DW_OP_stack_value)' failed.
...

Removed the naive code to check and add dwarf::DW_OP_stack_value in UpdateDbgValueInst() following @Orlando 's helpful advice and reverted to using DIExpression::prependOpcodes().

In D120168#3360280, @Orlando wrote:

I can clarify the comment. It's my understanding that non-empty DIExpressions must have a DW_OP_stack_value terminator. The expressions generated by the SCEVDbgValueBuilder objects do not have these terminators (as the expressions may be inserted into other expressions). If the pre-LSR expression was empty, once the complete salvaging expression has been generated, then the terminator must be appended.

My main confusion was in why the addition of stack_value was necessary in the patch but wasn't added before. I'd missed that you were previously using prependOpcodes with param stackValue=true, but are now adding the opcode yourself. And I agree that adding a stack_value is correct in this instance because you're generating implicit locations (though I think "non-empty DIExpressions must have a DW_OP_stack_value terminator" is not true in general?). In any case, sorry for the protracted review as a result of my misunderstanding!

prependOpcodes also accounts for inserting a stack_value opcode before fragment opcodes, which your implementation doesn't appear to handle. Would it be possible to rearrange the code to use prependOpcodes so that all cases are accounted for?

Attempt to make UpdateDbgValueInst() more clear when adding the DW_OP_stack_value terminator.

Correction to failed function renaming.

In D120168#3354027, @Orlando wrote:

I don't think there are "rules" about tagging patches as NFC (just put NFC somewhere in the title). It's definitely helpful to be able to see at a glance though - two examples: if an NFC patch is causing changes in output there's a bug, and if a patch is NFC it probably doesn't need any tests.

Nearly ready to approve but I'm still stuck on one thing (sorry if I'm just missing something obvious!).

Responded to review from @Orlando (Thanks!).

Removed unnecessary lit test contents as per review comments.
Fixed a comment typo in ScalarEvolution.h.

Applied clang-format.