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.