LGTM

So it seems the e_flags report that sram-ecc off, yet the code generated is for on.

Correct commit heading.

LGTM

Thanks for updating the subtarget features. I defer to @arsenm for the rest of the review.

LGTM

We are about to change the xnack and sramecc subtarget features to remove the "DoesNot" so I would suggest that FeatureDoesNotSupportUnalignedBufferAccess changes to FeatureSupportUnalignedBufferAccess.

LGTM

Sorry, just lots of questions:-)

LGTM except for minor :doc: reference comment.

Clarify that these are extensions to DWARF 5 and not as yet a
proposal.

Clarify these are extension to DWARF 5 and not as yet a proposal to DWARF.

[AMDGPU] DWARF proposal changes for expression context

Need to add clarification of how context is used in the evaluation of DWARF expressions and how evaluation relates to CFI information.

My understanding is this feature is equivalent to the OpenCL dynamic group segment allocation. The runtime would presumably implement it in a similar way.

[AMDGPU] Add loaded code object path URI definition to AMDGPUUsage

[AMDGPU] DWARF Proposal For Heterogeneous Debugging

Add introduction section.

[AMDGPU] DWARF For Heterogeneous Debugging

Adding minor updates as part of this review instead of creating a new review as the RFC email references this review and seems best to allow all comments within a single review.

LGTM. The call convention is still open to further refinement as more information is collected, but this appears to be an improvement so a good starting point.

LGTM

Thanks @echristo. I had been keeping all the updates on the same review as had provided the link to a number of non-LLVM reviewers so they can see the diffs more easily. @scott.linder had also sent out the link to the review in the RFC. Hope that seems reasonable.

[AMDGPU] DWARF proposal review feedback

Reopen for additional review feedback.

[AMDGPU] DWARF proposal review feedback

@nhaehnle my thinking was that the Vulkan memory model could be semantically represented in the LLVM IR by adding new memory orderings to make the distinction of the effective two sets of memory that the Vulkam memory model introduces. There needs to be memory orderings that only relates to the subset that Vulkan is requiring to be coherent. However, I have not followed the Vulkan memory model specification recently so not sure if it is still the same as when I was looking at it. What are your thoughts?

[AMDGPU] Move DWARF proposal to separate file

Reopening to update proposal to move it to a separate file.

In D70523#1981476, @echristo wrote:

Enthusiastic LGTM. :)

I would love to see this formally written up and passed along to the committee. Feel free to talk to me, Paul, Adrian, or a few others if you need or want any help.

[AMDGPU] Update DWARF proposal

Reopening to update proposal so more suitable to submit to DWARF site.

In D76881#1946594, @arsenm wrote:

In D76881#1946554, @scott.linder wrote:

In D76881#1946539, @arsenm wrote:

In D76881#1946435, @scott.linder wrote:

Right, but the placement of the meta instructions here can't affect change codegen, can it? Also, if we do need to skip the DEBUG_VALUEs it may also be that we would need to either update or drop them to avoid producing incorrect debug info. I'm not actually certain this is an issue though, but the situation I have in mind is if we pass an argument by-ref or on the stack, and in the callee we claim the value is in memory when the appropriate counts haven't been waited on yet.

Doesn't the placement of CFI matter relative to the real instructions?

Yes, and same for DEBUG_VALUE, which is where my uncertainty comes up. For CFI we absolutely need the directives at the beginning of the block to precede any real instructions, because they apply "on entry". For example, we reference the ABI return address registers here, and we can't have these be defined incorrectly until after the initial s_waitcnt. Conversely for DEBUG_VALUEs it may be that we are constructing them assuming they can reference memory locations for arguments that are only valid after the initial s_waitcnt. So I think we need to handle the two cases distinctly.

ok

Added @mjbedy to review.