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.