Thanks! I'll land this shortly.

In D142552#4083939, @clayborg wrote:

As long as there are no regressions in the test suite this looks good to me

In D142556#4142682, @dblaikie wrote:

Reckon this is worth a go - maybe wait for @aprantl to sign off too, though.

Address comment and fix the patch for non-assert builds.

In D142556#4086698, @jmorse wrote:

I've run out of time to read the code this week, but it sounds like a good approach to me, I'll give my debugger colleagues a prod about it too.

In D142556#4126647, @dstenb wrote:

In D142556#4124049, @dblaikie wrote:

compile-time-tracker results?

I have gotten results now, but I will not have time to look at them tonight: https://llvm-compile-time-tracker.com/compare.php?from=f7b10467b63f09ab74e67d4002b3e11601091882&to=d200692f433fbc5b8edaa68e92ab04124d0c7e6a

In D142556#4124049, @dblaikie wrote:

compile-time-tracker results?

In D142556#4119696, @dblaikie wrote:

I'm a bit confused by all these reports - at least with my bloaty locally, "+100%" would mean the section doubled in size (100% growth) but it shows "+100%" even for a section that shrunk (.debug_line_str) slightly... Confusing.

but the overall numbers still sound OK.

@dblaikie, @ayermolo: I have now ran measurements on an otherwise unused machine to get rid of the noise.

In D142556#4103178, @dblaikie wrote:

Numbers look fairly good - wonder if you could get this run through the LLVM perf tracker to get some more stable/precise numbers?

For the RelWithDebInfo opt 8.0 binaries that was mentioned in the previous comment the number of inlined subroutine DIEs increased by ~2.8%:

In D142556#4086340, @dblaikie wrote:

In D142556#4085390, @dstenb wrote:

In D142556#4080303, @dblaikie wrote:

Oh, neat idea - but I wonder what it does to the DIE tree, rather than just the line table? Do we end up with 3 inlined_subroutines? (one for each of the original calls, if tehy have any unique instructions after inlining, then one for this sythesized call? I guess that's not strictly worse - the instructions were from some sort of inlining... )

Yes, we will end up with 3 inlined_subroutines in the example above (as both original calls have unique instructions):

How's the performance?

When building a Clang binary with this patch to see how often the new case occurs I encountered a bug where the BRIt iterator was invalidated due to further insertion into BLocs. I have fixed that and added a unit test.

In D142556#4080303, @dblaikie wrote:

Oh, neat idea - but I wonder what it does to the DIE tree, rather than just the line table? Do we end up with 3 inlined_subroutines? (one for each of the original calls, if tehy have any unique instructions after inlining, then one for this sythesized call? I guess that's not strictly worse - the instructions were from some sort of inlining... )

As far as I can tell it is valid DWARF to inline at line 0.

First time looking at LLDB, so I had trouble finding a suitable way to create a reproducer for this. I would be happy to change to another type of lit test if that is more suitable.

Interesting patch series! Just adding some stylistic comments whilst reading through the code.

In D114034#3136057, @mattbeardsley wrote:

@dstenb - wanted to check with you too to make sure that this change to pr37091.cpp seems like it won't interfere with the original intent of the test (https://reviews.llvm.org/D45686 )

The lingering file issue you'd fixed seemed to be independent of any particular clang-tidy check, but if not, hopefully it can at least get away with not relying on the top-level .clang-tidy anymore!

Thanks for the reviews! I'll land this shortly.

In D97667#2730798, @zhuhan0 wrote:

In D97667#2725626, @dstenb wrote:

With this patch we got the following assertion:

bool llvm::APInt::operator==(const llvm::APInt &) const: Assertion `BitWidth == RHS.BitWidth && "Comparison requires equal bit widths"' failed.

in LoopIdiomRecognize::processLoopMemCpy() at the following comparison:

// Check if the load stride matches the store stride.
if (StrIntStride != LoadIntStride && StrIntStride != -LoadIntStride)
  return false;

for a memcpy done between two address spaces with different pointer sizes.

I don't have a upstream reproducer ready for this, but I'll see if I can create one.

The new version should fix the assertion I think. Let me know if you still see it.

With this patch we got the following assertion:

Looks good. Just a small nit about the test.

Hi, @arichardson! What is the status for this patch?

In D90020#2477208, @sdesmalen wrote:

In D90020#2455122, @dstenb wrote:

I'm sorry for chiming in so late here! I have a comment about the prependOffsetExpression target hook.

Thanks for the suggestion, you made a good point that this interface was error prone when new flags are added in the future. I've updated the patch and added the virtual interface getOffsetOpcodes alongside prependOffsetExpression (made non-virtual and now handles the Deref, Stack/Entry Value). Let me know if this is how you imagined it.

I'm sorry for chiming in so late here! I have a comment about the prependOffsetExpression target hook.

In D76146#2438388, @mib wrote:

Hi @dstenb, any update on this ? I tried to look for DW_OP_LLVM_convert_generic in the codebase but couldn't find any occurence ... Thanks.

Thanks!

As both the emission of the IMPLICIT_DEF instructions in SelectionDAG, and the resolving of those instructions in "Process Implicit Definitions", is target independent code, I think it would be sufficient with only keeping one of these test cases, but I would be fine with landed this with all five.

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.

What is the status of the expensive check failure? It has been present for five days now. Should we revert this patch until that is resolved?

In D91153#2415351, @mkazantsev wrote:

Please move the test out of MIPS-specific into general test directory if it's possible. Otherwise, LGTM.

Use X86 reproducer instead.

I took the liberty to add some review comments whilst familiarizing myself with the code.

In D91153#2393018, @TaWeiTu wrote:

In D91153#2385479, @dstenb wrote:

I'm not very familiar with the IndVars pass, so I have not idea if this is the correct way to solve this.

This seems correct to me, but I'm not familiar with IndVar either, so maybe someone else should take a look.
However, I think it would be better if we can return false in this case (i.e. if all PHI nodes created in rewriteLoopExitValues are dead).
But I don't know whether there's some edge case that forces us to return true here.

Thanks! This looks good to me.

I'm not very familiar with the IndVars pass, so I have not idea if this is the correct way to solve this.

In D89613#2376068, @vtjnash wrote:

The .cfi_startproc directive is part of the unwind information (eh_frame), and is not debug info, so that seems like a possible bugfix even? I'd expect that observation to be consistent with the goal of this PR, though I didn't see specifically where that pass was being affected in a cursory glance back through the PR.

In D53758#2336719, @dexonsmith wrote:

I totally lost track of this :/.

The patch looks correct, thanks very much for tracking this down. This LGTM if you add an explanation to the testcase of exactly what it’s testing (I worry it’s not obvious for someone trying to update the test later).

Rebase, and update comment in test case.

This seems to have broken the expensive-checks build bots:

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?

Some minor code style comments while I look into this.

I added a comment about eliminateTrunc to the commit message. I'll see if I can create a reproducer for that, and if so, I'll upload a revision for that.

I'm equally fine with doing the Implicit flag change here, or for someone to do it in a separate patch.

Sorry for a piecemeal review from my part!

Thanks for the review!

Just some drive-by nits while familiarizing myself with this patch series.

Continue using a bool return instead of a tri-state.

Thanks, and sorry for overlooking the comments!