Thanks for taking a look folks.

I think wait for someone with more experience in this area to give approval, but aside from the outstanding comments, LGTM.

Cray/HPE is motivated to see this patch land. My confidence is low in this area, but I don't see any review comments that are fatal. Any reason we can't move forward with this change?

In D87147#2288103, @dblaikie wrote:

In D87147#2288024, @aprantl wrote:

I think the comment in https://reviews.llvm.org/rL349065 describes it quite well:

The DIFile used by the CU is special and distinct from the main source
file. Its directory part specifies what becomes the DW_AT_comp_dir
(the compilation directory), even if the source file was specified
with an absolute path.

So while the CU links to a DIFile it's more a storage mechanism for DW_AT_comp_dir and not a "file" in the normal sense and thus should be treated special. (And we might want to just store a string instead to avoid this confusion in the future.

Fair points.

@umesh.kalappa0 is there specific observable behavior of the resulting DWARF you were trying to address (I seem to recall/think there was)? Or only the quirky/strange IR representation?

@dblaikie ,like stated in https://bugs.llvm.org/show_bug.cgi?id=47391 ,we are trying to address the specific behavior, where the debug_line referring to two different file index's (1 and 2).

@StephenTozer @dstenb Thanks for your comments! I'll try to change the approach (by putting some extra context), so we might end up changing this one (a bit).

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

LGTM. I think the Implicit flag idea might be able to be folded into this patch without much issue, but it's not urgent and can definitely be shifted to another patch (or even just left as a TODO).

hmm, possibly more words in the patch description (& IR documentation). of this and the previous patch - what is implicit_pointer V explicit_pointer? Maybe some examples in the patch description and IR documentation about how they could be used?

Updated changes for latest master (there may be some lurking unseen errors, still trying to flush any out), addressed some review comments. Right now, I think the only thing holding this patch up is the discussion on the llvm-dev mailing list about using DBG_VALUE_LIST as the default for all debug values. If anyone on this review has any input for that, please go ahead and reply to the chain - I think that the latest proposal I put forward should satisfy everyone's requirements, but I don't want to push ahead without being sure of that.

LGTM

LGTM; I guess it'd be better if SCEV exposed a helper method, rather than this patch having to expose one, but that's well out of scope.

• addressing comment

• addressing comments

In D87233#2288809, @dstenb wrote:

Sorry for a piecemeal review from my part!

No problem, the tempo works for me. :) Thanks!

In D87494#2285378, @jmorse wrote:

This is looking good, a few nits inline. Some slightly broader questions: do you know if there's any risk of debug-info affecting decisions made by SCEV, i.e. causing codegen to change when compiling -g? I don't have any reason to believe that could be the case, but it's a fear in the back of my mind.

Fixed most of the review remarks.

Sorry for a piecemeal review from my part!

LGTM with moderate confidence. Does anyone see a reason that this shouldn't land?

LGTM with moderate confidence. Does anyone see a reason that this shouldn't land?

In D87147#2288024, @aprantl wrote:

I think the comment in https://reviews.llvm.org/rL349065 describes it quite well:

The DIFile used by the CU is special and distinct from the main source
file. Its directory part specifies what becomes the DW_AT_comp_dir
(the compilation directory), even if the source file was specified
with an absolute path.

So while the CU links to a DIFile it's more a storage mechanism for DW_AT_comp_dir and not a "file" in the normal sense and thus should be treated special. (And we might want to just store a string instead to avoid this confusion in the future.