Add [AMDGPU] to the title.

LGTM

LGTM

LGTM

Added comments about the values used.

Sorry for delay, thanks for doing this!
This is LGTM, except it needs a rebase: ISD::SCALAR_TO_VECTOR for Vec16 has changed since this was created.

Renamed 'Fast' used in the file to 'RelativeSpeed'.

Rebased.

Fixed rebase artifact with misplaced comment.

In D114644#3518463, @piotr wrote:

I am strongly in favour of this change. On our target, the sgpr improvements that manifest themselves in reduced sgpr spilling outweighs the code size increase.

LGTM

Rebased.

LGTM with formatting fixed.

ping

LGTM

Code was submitted as a part of D124884. Test was submitted separately.

LGTM

Added comment about offset operand.

Changed asserts to cannot select.

Changed asserts to cannot select.

In D106408#3516718, @Carrot wrote:

@rampitec, any follow up on this patch?

Struct version is now handled by the parent patch D124884 itself.

Switched to direct select which allows to use 2 separate memory operands.
The patch now handles both raw and struct intrinsics.

Return false from getMemOperandsWithOffsetWidth() instead of checking mem operand.

Return false from getMemOperandsWithOffsetWidth() instead of checking mem operand.

ping

Removed support for wide than DWORD ops. See D125409.

Removed support for wide than DWORD ops. See D125409.

I do not understand how may this work. The LDS address has the component TID * 4. I.e. neighboring lanes shall overwrite each other. I can only imagine a scenario with a very specific EXEC mask where it might be different.

Do not split voffset because inst_offset is applied to both VMEM and LDS address and voffset is not. Add a separate operand instead.
Same as in parent change D124884.

Do not split voffset because inst_offset is applied to both VMEM and LDS address and voffset is not. Add a separate operand instead.

In D125126#3498596, @arsenm wrote:

In D125126#3498084, @rampitec wrote:

A potentially better alternative is to use gfx940 names with _LDS_ in the mnemonic instead of a modifier. This is logically a different opcode anyway. The only downside it is not compatible with the documentation and sp3. But then it was not implemented before and therefore not used, so there shall be no compatibility problem on practice. Well, it will also be different from MUBUF. Given the difference in both semantics and addressing mode I personally would prefer it to be different opcodes. At a pseudo level it is certainly easier to have separate ops for this.

Preferences?

It's probably better to have separate opcodes. In general I think the way we try to force all of these subtarget changes onto the same generic pseudos is more trouble than it's worth. It requires more and more code to verify and make use of the features, and it would be cleaner to move towards separate instruction definitions per subtarget

One more thing to note: it is already incompatible with sp3 because we prohibit unused vdst, while sp3 enforces it.

A potentially better alternative is to use gfx940 names with _LDS_ in the mnemonic instead of a modifier. This is logically a different opcode anyway. The only downside it is not compatible with the documentation and sp3. But then it was not implemented before and therefore not used, so there shall be no compatibility problem on practice. Well, it will also be different from MUBUF. Given the difference in both semantics and addressing mode I personally would prefer it to be different opcodes. At a pseudo level it is certainly easier to have separate ops for this.

LGTM

In D125117#3497537, @foad wrote:

I thought this was a GFX9 feature?

As far as I understand this requires a whole program compilation and will not work with late linking?

In D124884#3496841, @ramjana wrote:

Just to be clear , Is your expectation that intrinsic user to save and restore m0 before calling buffer_load lds intrinsic?

• Removed the overload and added i32 %size operand instead.
• LDS pointer is i8 addrspace(3) now qualified with the address space.
• Rebased on the change to handle hazards between m0 initialization and these operations.

LGTM, but please wait for Austin too.

In D124884#3490745, @arsenm wrote:

In D124884#3490731, @nhaehnle wrote:

What's wrong with the idea of an i32 imm %size argument? That seems to me more in line with the philosophy of caring less about types.

I'd prefer to keep any intrinsics that look like a load or store to look more like the regular load or store instructions. All of these arbitrary immediate parameters are uglier (e.g. the memory ordering arguments that don't actually work on some of the atomics)

In D124884#3490643, @arsenm wrote:

In D124884#3489967, @rampitec wrote:

To confirm: is that OK to add yet another imm to the end of operands of the intrinsic to select a byte size? And then remove the overload. If yes I will do it tomorrow.

There's the elementtype attribute for this case which some arm intrinsics seem to be using. Not sure how you're supposed to define an intrinsic to use it though

In D124884#3489967, @rampitec wrote:

To confirm: is that OK to add yet another imm to the end of operands of the intrinsic to select a byte size? And then remove the overload. If yes I will do it tomorrow.

To confirm: is that OK to add yet another imm to the end of operands of the intrinsic to select a byte size? And then remove the overload. If yes I will do it tomorrow.

In D124884#3489729, @arsenm wrote:

Why does this need an intrinsic? I thought the whole point of the LDS DMA thing was an optimization the backend would perform and doesn't need to be exposed directly

It will also need to be rebased on top of D124550 to handle hazard between M0 initialization and LDS DMA.