LGTM

In D30970#701733, @arsenm wrote:

In D30970#701589, @yaxunl wrote:

LGTM.

Do we have a general guideline what should be in metadata and what should not? Do we assume that end user should be able to manually craft the metadata when they write the ISA assembly by hand?

I'm not sure why we need to encode some of these static hardware properties into it. Why doesn't the runtime/debugger know these already?

Just a wild thought:-) Since address spaces and memory scopes are both target defined concepts, it seems best for the target to define the enumeration of the ones it supports. Would it make sense for the data layout to not only give the properties of the address spaces, but also give their textual names, together with the textual names of the memory scopes supported by the target? The memory scopes would include the memory scope to use for singlethread and crossthread.

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

Also need to use the trap handler ABI query to see if there is a trap handler, and if there is add the TRAP_HANDLER_SGPR_COUNT to the number of SGPRs budgeted for the wave in determining the number of waves per EU calculation. TRAP_HANDLER_SGPR_COUNT is 16 for GFX6 onwards.

Feedback on overall pass.

LGTM

LGTM

I would argue that even without (2) there is a case for using standard LLVM atomic instructions as it makes it possible to generate the same IR for a given language regardless of whether the target supports scopes. It seems CLANG is moving towards generating LLVM IR atomics directly rather than calls to built-ins so this approach would support that. Currently LLVM already supports two scopes. It also makes code generation much simpler as the existing machine instructions can be used and so avoid creating a large number of pseudo instructions. The patches D24577 and D24623 that are under review are pretty simple and do that. There would be a lot more code if intrinsics had to be used.

The old R_AMDGPU_ABS32_LO and the new R_AMDGPU_ABS32 are in fact the same thing. The & 0xffffffff is implicitly done because the result of the R_AMDGPU_ABS32 is a word32 not a word64. So putting in the & 0xffffffff is redundant. The other ABI documents (such as for the x86) do not put in the & 0xffffffff so it seemed best to follow their conventions. R_AMDGPU_ABS32_HI and R_AMDGPU_ABS32 both return 32 bits as they are both defined as word32. They differ in that R_AMDGPU_ABS32_HI takes the address and shifts it right by 32 bits which effectively means that the top 32 bits of the 64 bit address are returned, not the bottom 32 bits.