In D68578#1700652, @tra wrote:

In D68578#1698864, @t-tye wrote:

From a source language point of view, the device function comprises the code that is launched as a grid. We need this fact to be present in the symbols used. Only the device function should have a symbol name matching the mangled name of the device function.

What do you have in mind when you use 'symbol name' here? Is that a symbol as seen by linker? If that's the case, do host and device share this name space on AMD GPUs? In case of CUDA, linker symbols are per-target (i.e. host and each GPU have their own spaces), so they never clash, but the kernel names must have identical mangled name on host and all devices, so the host can refer to the device-side kernel when it needs to launch it.

In D68578#1697898, @tra wrote:

In D68578#1697851, @yaxunl wrote:

In D68578#1697822, @tra wrote:

Could you elaborate on how exactly current implementation does not work?

I would expect the kernel and the stub to be two distinct entities, as far as debugger is concerned. It does have enough information to track each independently (different address, .stub suffix, perhaps knowledge whether it's device or host code). Without the details, it looks to me that this is something that can and should be dealt with in the debugger. I've asked the same question in D63335 but I don't think I've got a good answer.

HIP debugger is a branch of gdb and the changes to support HIP will be upstreamed. When users set break point on a kernel, they intend to set a break point on the real kernel, not the device stub function. The device stub function is only a compiler generated helper function to help launch the kernel. Therefore it should have a different name so that it does not interfere with the symbol resolution of the real kernel.

I would agree that having distinct names for the device-side kernel and it's host-side stub would probably make things easier for debugger.
However, debugger does have access to mangled names and does see the '.stub' suffix in the mangled name. I don't understand why it can't be considered to disambiguate between the kernel and the stub?
I'm clearly missing something here. Is there a chance to get someone from the debugger team to chime in on this review directly?

Also, I would not agree that they intend to set a break point on the real kernel is the only scenario. E.g. quite often when I debug CUDA stuff, I do only care about host-side things and I do want to set breakpoint on the stub, so I can check kernel call parameters as they are passed to the kernel. It would be great if there were a way to explicitly tell debugger whether we want host-side stub or the kernel without having user to know how particular compiler transforms the name. For the user both entities have the same name, but distinct location and there should be a way to express that in the debugger.

How does this work for EXEC mask?

In D59008#1442014, @dblaikie wrote:

In D59008#1441903, @t-tye wrote:

LGTM

Do we know the state of split DWARF and DWARF compression for DWARF 5 (compared to DWARF 2)?

State of them in what sense? Compression is pretty orthogonal to any DWARF version - it's more about the container (ELF, etc) you use. Split DWARF is non-standardly supported in pre-v5, and I think it's functioning in the standards conformant v5 mode too.

Do we know the state of split DWARF and DWARF compression for DWARF 5 (compared to DWARF 2)?

LGTM with comment update.

LGTM

To clarify, I am saying that the stub does have a different name since it is conceptually part of the implementation of doing the call to the device function implementation, and is not in fact the the device function being called itself. However, when we generate code for a function that is present on both the host and device, both copies of the code are for the same source level function and so can have the same symbol name (which was a question that was asked).

Yes this relates to supporting the debugger.

LGTM

In D54228#1302715, @nhaehnle wrote:

Ping?

I think the remarks by @t-tye point to a potentially useful optimization, but that should not be part of this patch.

LGTM

In D54228#1292786, @nhaehnle wrote:

In D54228#1290997, @t-tye wrote:

This is sufficient, because whenever only one event of a count type is

pending, its last time point is naturally the upper bound of all time
points of this count type, and when multiple event types are pending,
the count type has gone out of order and an s_waitcnt to 0 is required
to clear any pending event type (and will then clear all pending event
types for that count type).

Just wondered if can do better than using 0. Instead can the lowest count be used as this should be sufficient to ensure all out-of-order events in this have happened? I had discussed this with Bob at one time.

Hmm, how would that work? What lowest count are you referring to? For example, if lgkm has both in-flight SMEM read, and in-flight LDS, we could either have all SMEM read finish first or all LDS finish first.

Something that we could do is a more finely-grained tracking of in-order events. For example, if we have both in-flight SMEM and in-flight LDS, and we need to wait for the second-to-last LDS, then in fact we could do an lgkmcnt(1) wait -- because if the counter reaches 1 or less, the second-to-last LDS must have returned. After the lgkmcnt(1), we still need to conservatively assume that any event type that was previously in-flight may still be in-flight, so this patch here is compatible with such a more finely-grained tracking.

I think the finer-grained tracking could be achieved by introducing separate timelines for each event type: currently we only have timelines by counter. Anyway, it'd be a separate change, mainly for the benefit of mixing LDS and SMEM I think.

LGTM

This is sufficient, because whenever only one event of a count type is

pending, its last time point is naturally the upper bound of all time
points of this count type, and when multiple event types are pending,
the count type has gone out of order and an s_waitcnt to 0 is required
to clear any pending event type (and will then clear all pending event
types for that count type).

LGTM

In D53153#1288127, @rjmccall wrote:

In D53153#1288112, @rjmccall wrote:

But do you want to support *dynamically* linking object files? Because that's what visibility is about.

To be specific, if you don't have multiple levels of linking — doing a slower and relatively more expensive link to form a self-contained unit of code distribution, then doing a faster link to form a runnable program from multiple independently-distributed such units — then visibility isn't really doing anything for you.

Summary needs updating as now only being done for kernels and not namespace scope variables.

Should the v2 version of the metadata be moved to a separate section rather than be deleted since it is still generated when the v2 code object format is requested?

LGTM (scheduler can be done in separate change list)

Does the assembler need any support for this? For example, does the .amdgcn_target directive need to accept the +sramecc and ensure the e_flag is set accordingly?

LGTM

LGTM

LGTM

LGTM

Another word commonly used across languages is "offload".

LGTM

Now this is implemented it may be worth converting the memorylegalizer tests from MIR to IR tests.

Abandon as duplicate of D47549.

Rename note record enmerator from NT_AMD_AMDGPU_* to NT_AMDGPU_* to match the vendor name change from "AMD" to "AMDGPU".

LGTM

LGTM

In D47504#1118412, @t-tye wrote:

In D47504#1117336, @arsenm wrote:

Should add some tests for this GDS support. Using a pure MIR test you can bypass the fact that we don't codegen it yet

Would it be ok to do that as a separate patch? Would like to add code selection patterns to handle lds and gds atomics and then use that to make an llvm ir test.

Add MIR tests for local and region address spaces.

In D47601#1120916, @nhaehnle wrote:

In D47601#1120903, @nhaehnle wrote:

What is this actually needed for? Having a relative relocation in a data segment doesn't seem that useful?

Never mind, I see it in the other commit. Could you please update the table of relocation types in AMDGPUUsage.rst? It has an R_AMDGPU_RELATIVE64, I don't know what that's about, but it doesn't mention R_AMDGPU_REL64.

Further minimize MIR tests (thanks @rampitec ).

In D47504#1117336, @arsenm wrote:

Should add some tests for this GDS support. Using a pure MIR test you can bypass the fact that we don't codegen it yet

Reduced size of mir tests.

Update for @rampitec review comments.

LGTM

In D47261#1112309, @arsenm wrote:

Needs a test, preferably the full set of AA checks with 32 bit constant

In D47261#1112296, @hakzsam wrote:

In D47261#1109743, @arsenm wrote:

Needs a test, preferably the full set of AA checks with 32 bit constant

Sure., but can you give me more details about what you want? Is there already an example that I could start from?

Unfortunately it looks like the commit that added this didn't actually add a proper test for this although I thought there was one that would be easy to add to. I would like there to be a test that purely tests the results of alias queries, like those found in test/Analysis/BasicAA or test/Analysis/ScopedNoAliasAA/basic.ll

LGTM

In D47370#1112733, @arsenm wrote:

In D47370#1112729, @rampitec wrote:

Are we sure that is what RT(s) do?

It doesn't really matter if it does or not, since we're now requesting the larger allocation

LGTM Thanks:-)

LGTM except for minor suggestions.

LGTM

LGTM

@timcorringham ping

I believe the metadata does have the actual (ie non-granulated rounded up value) register count. So the runtime could access the information from there.

LGTM

In D44718#1047028, @b-sumner wrote:

LGTM, but I'd rather use '+' instead of ',' for the features.

Change syntax for target features to be concatenated to Processor using a plus prefix for each target feature.

Change target feature element of the target identification string to be comma separated so hyphens only delimit the elements.