Thanks @djtodoro for looking at this. Sorry for this really delayed response!

Hi, sorry for the slow update.

As far as I understand it I don't think it is safe to move CFI_INSTRUCTIONs
around like this. CFI_INSTRUCTIONs describe the effect of specific instructions.
We encode where this 'description' starts by the position of the CFI_INSTRUCTION
in the instruction stream. So, if you move the CFI_INSTRUCTION like this you
are changing the meaning.

Thanks for taking a look @aprantl.

Missed one of djtodoro's comments, sorry for the spam!

+ Rebase.
+ Address comments from @djtodoro.

+ Update dexter test for @jmorse.
+ Small additional change to print().
+ Rebase.
+ Rephrase a small part of the summary.

Adrian said:

Thanks! Let's cycle back to this after the llvm-dev thread that @dblaikie started has been resolved.

Thanks for the review, committed in 2de23c8364b.

Hi @alok. I haven't been following the DW_OP_implicit_pointer discussion too closely, so just some inline nitpick comments/questions from me.

LGTM -- though having given only one LGTM before I'd feel more comfortable waiting for +1.

@aprantl wrote https://reviews.llvm.org/D70318#inline-635016:

Thanks all for the initial reviews.

+ Added reviewers

+ Improve some comments
+ rename getter/setters as Adrian requested
+ remove some whitespace changes

More Info about this patch: As far as LDV is concerned undefs and 'blockers' are semantically very similar. However, blockers are not emitted (as DBG_VALUES) because it is unnecessary: They are only added to UserValues with overlapping fragments for any given def, and a def implicitly terminates preceding defs with overlapping fragments [0].

In D68945#1714323, @jmorse wrote:

In D68945#1712448, @Orlando wrote:

I'm working in this area for pr41992 so I noticed a couple of things (inline comments). I am happy to just add the changes I'm suggesting into my own upcoming patch if you'd like?

Those both sound legit, please do!

I'm abandoning this because I've created the WIP revision D70121 which handles fragment overlaps.

Hi @bjope thank you for pointing this out and providing a workaround. I'll test out your change and investigate further.

I'm working in this area for pr41992 so I noticed a couple of things (inline comments). I am happy to just add the changes I'm suggesting into my own upcoming patch if you'd like?

After timing some more self-hosts it looks like my original numbers which looked
too good to be true sadly are just that. The self-host asan build time reduction
seems correct (-3.5%) but the non-asan builds are much less impacted then first
thought (about -1%). These numbers are more in line with my original expectation.

Addressed comments and fixed Summary (previously it referred to
LiveDebugValues.cpp instead of LiveDebugVariables.cpp).

The numbers are much better than I expected but I haven't been able to prove
myself wrong after a bunch of testing. So, I've put this patch up for the
community's expert eyes.

I don't feel confident enough in this area to give an official LGTM but If I understand it correctly the patch seems reasonable: move loop metadata from old to new latch when a loop header is split.

Thanks for the review!

Thanks, I've now resubmitted this patch (1251cac62af5).

LGTM. Thanks for doing this!

In D60831#1546469, @aprantl wrote:

There is one thing I don't understand:
It looks like !680 is only used in the !llvm.loop !679 metadata. How can that trigger a !dbg attachment points at wrong subprogram for function verifier failure?

Hi Adrian.

No problem at all - reverted with rGa94715639619. I'll take a look at this next week, thank you for the info.

The major change to this previously accepted patch is the modification of "bcmp-debugify-remarks.ll" and "inlined-argument.ll".
Minor changes include spelling corrections in comments and removing some superfluous debug data from the new tests.

The inlined loop metadata part has been separated into D61933 (rL361149) which this patch is now based on.

Resubmitted in rL361149.

Reverted with commit 95805bc425b because armv8 build bots are failing.

Address suggestions and add some const correctness.

This update addresses the problems with the original patch that caused the built bot failures
(sorry about that!).

Hi, I added a few inline comments, mostly just nits on style.

My take-away from the discussion was this: It is desirable to map the instructions to something in the loop (e.g., not line 0), unless doing so will provide confusing information to the mapping that PGO uses to optimize the relevant branches. Am I correct in saying that this latter issue is of minimal concern in this case?

A summary of the discussion so far:

Are the function attributes in the test necessary? Watching other reviews leads me to believe these are undesirable. Apart from this it LGTMy untrained eyes (I'm new to reviewing!).

In D60831#1470677, @hfinkel wrote:

In D60831#1470585, @Orlando wrote:

In D60831#1470520, @hfinkel wrote:

How will this appear to profiling tools using PC sampling and using debug info to map the PC samples back to line numbers in the code?

I brought this up in some offline discussions which all concluded that the impact on profilers would be small and the trade off for better debugging is worth it. The impact on profilers seems like it would be small because the middle block is only visited once after running through the vectorized loop.

I am glad you asked because this concern gives rise to an argument for giving the middle block instructions line 0 instead, and i am interested in hearing other's opinions.

Interesting. The middle block just has the check for whether or not we need to run the remainder loop, right? I can definitely see this as kind of latch-like.

@jmellorcrummey , do you have an opinion on this?

Sorry, I missed a couple of failing tests. I've fixed them and updated the diff.

In D60831#1470520, @hfinkel wrote:

How will this appear to profiling tools using PC sampling and using debug info to map the PC samples back to line numbers in the code?

I've made the suggested changes.

Updated a comment with some more info as vsk suggested.

The test has been updated to check for the expected instructions.

I've updated the test to include the source and commands to generate the IR it uses. Please note that I stripped out the call void @llvm.dbg.* lines from the test file by hand so any regenerated IR will look more cluttered.