In D103225#2803733, @rampitec wrote:

In D103225#2792240, @rampitec wrote:

In D103225#2785687, @rampitec wrote:

You probably need to wrap all prologue LDS stores into a block to execute it only from lane 0 and add a barrier after. @t-tye correct me if I am wrong.

But, I remember that we had decided to avoid barrier here, and instead just make sure that each thread within each wave execute the store instructions? In anycase, let me clarify it with @t-tye and @b-sumner.

I do not remember, but probably we can omit it since it is a singe store readonly memory. Anyway a confirmation from @t-tye would be nice.

For the record, the agreed way is to do a store from lane 0 of each wave and follow with a wave barrier.

Add feedback from @dp .

Can you review D102859 instead? I will add your change to that as well.

In D102691#2767846, @JonChesterfield wrote:

I like the direction. Could we hold it for a day or so? I'd like to check through the uses of the kernel name to see if there's a missing edge case, or if we can simplify this a step further.

It looks like the msgpack data always contains the foo and the foo.kd strings, under different keys. I wonder if that's something we can rely on the compiler emitting.

In D102708#2767022, @t-tye wrote:

This needs AMDGPUUsage to also be updated that these values are now reserved.

This needs AMDGPUUsage to also be updated that these values are now reserved.

LGTM

The documentation parts LGTM.

I suspect there is something in hardware that rounds LDS allocation up to a boundary, so as long as the kernel looks like it uses some non-zero amount of LDS, the out of bounds read hits in the allocated region.

Added @b-sumner to review.

In D102177#2750461, @madhur13490 wrote:

In D102177#2750256, @sebastian-ne wrote:

In D102177#2750094, @madhur13490 wrote:

In D102177#2748278, @arsenm wrote:

We have another proposal we were working on to rearrange these a bit differently. We need to account for a few more inputs in the layout

As long as this remains in GFX land, we should be fine with it because our new proposal is for compute only (as of now).

I would like to keep the same calling convention in compute and graphics. At least regarding the stack pointer and others, because I don’t see a compelling reason to diverge even more. Actually, I’d like it if they were more common than they are now, because we implement some things twice at the moment.
The compute proposal should work just fine; if we move the stack and frame pointer, we end up with the same benefits as in this patch. I commented on the internal proposal for this (I hope I found the right one?).

Well, then you're saying unification of both ABIs and it is not discussed thoroughly internally. The layout needs to be documented and get reviewed internally before we can proceed with this patch.

LGTM

In D100404#2701630, @palves wrote:

Sure, it will work for me, though FWIW I think it's the worse option of the 3 potential solutions, and I'll be puzzled if we end with that UI.
Can anyone else speak up and state your opinions, please?

LGTM

Documentation LGTM

LGTM

In D96336#2553493, @sebastian-ne wrote:

As an aside, if we are moving to using flat scratch in the main, is it possible to replace most of this with s_scratch_store / s_scratch_load and avoid the need for an VGPR entirely?

That would make sense, but it feels like s_scratch instructions got removed in newer hardware.

LGTM

Address review comments.

Address review comments.

What was the decision on the philosophy on who should be doing unbundling in general? How is it known that it is sfe for the memory legalized to do this unbundling? Is the policy documented in some AMD GPU design page?

In D98940#2637456, @foad wrote:

In D98940#2637450, @t-tye wrote:

In D98940#2637447, @foad wrote:

See D72737 and D91048 for the history of that. I'm not saying it's the right place to do it. The point of the current patch is to help us move away from that, i.e. to do less unbundling in SIMemoryLegalizer.

I do not think SIMemoryLegalizer should be doing any unbundling as it is conflating tasks. So will this direction manage to eliminate it?

No, not entirely. D72737 added some unbundling (only of bundles containing loads or stores) and D91048 added some more (all bundles). The intention here is just to undo the D91048 part.

Why would SIMemoryLegalizer know it is correct to do the unbundling? Presumably the instructions are in a bundle for a reason? How does SIMemoryLegalizer know that that reason is no longer necessary? Is SIMemoryLegalizer meant to know the passes hat run after it and what their needs are? Is there a point in the compilation where the need for these bundles to exist disappears? If so shouldn't that be the point the unbundling is done? It feels uncomfortable to put this kind of things in passes as it makes things very brittle. If someone made changes to the bundling would they be aware that SIMemoryLegalizer is doing this?

Don't ask me! I'm just trying to explain the status quo. I am also uncomfortable with the idea of bundles being unbundled or modified in general, though I can see that it would be useful to be able to bundle some stuff in one pass and then later unbundle "my" bundles only -- but at the moment there is no robust way of distinguishing "my" bundles from anyone else's.

Anyway I think you need to take this up with @rampitec who wrote D72737.

In D98940#2637447, @foad wrote:

See D72737 and D91048 for the history of that. I'm not saying it's the right place to do it. The point of the current patch is to help us move away from that, i.e. to do less unbundling in SIMemoryLegalizer.

I do not think SIMemoryLegalizer should be doing any unbundling as it is conflating tasks. So will this direction manage to eliminate it?

No, not entirely. D72737 added some unbundling (only of bundles containing loads or stores) and D91048 added some more (all bundles). The intention here is just to undo the D91048 part.

In D98940#2637391, @foad wrote:

In D98940#2637384, @t-tye wrote:

In D98940#2637166, @foad wrote:

Currently these are unbundled in SIMemoryLegalizer.

Why is the SIMemoryLegalizer the right place to do the unbundling? Why would the memory SIMemoryLegalizer know that it has "permission" to do that? The SIMemoryLegalizer was intended to do the single job of expanding the atomics semantics.

See D72737 and D91048 for the history of that. I'm not saying it's the right place to do it. The point of the current patch is to help us move away from that, i.e. to do less unbundling in SIMemoryLegalizer.

In D98940#2637166, @foad wrote:

Currently these are unbundled in SIMemoryLegalizer.

I vaguely remember that clang needed to know what code object it was going to request as it used that to either validate other options, or change the format of other passed cc1 options. If that is true, then I am not sure the defaulting approach works as clang will not know what the backend will be defaulting to. @yaxunl can you remember this?

I have no opinion, just making an observation and defer to @kzhuravl .

An example usage is to run a large part of the gdb test suite on the GPU. The tests normally run on the CPU, but can also be made to run on the GPU within a test harness that emulates the necessary environment. For that to work the variables need to be forced to be device. The issues of concurrency do not happen due to the nature of the environment. Porting or modifying the entire test suite is not particularly viable.

Discussed offline.

What other debuggers do you have in mind? There is the rocgdb and the UMR hardware debugger. Do you know of others for AMDGPU? If non-HSA targets would like to support a debugger it would make sense that the also add support for llvm.debugtrap as well:-)

The expected use case is for frontends to insert this into
shaders that are to be run under a debugger. The shader can
then be resumed or single stepped from the point of the call
under debugger control.