Page MenuHomePhabricator

[AMDGPU][MC][GFX8] Added BUFFER_STORE_LDS_DWORD Instruction
ClosedPublic

Authored by dp on Mar 1 2018, 9:13 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

dp created this revision.Mar 1 2018, 9:13 AM
artem.tamazov requested changes to this revision.Mar 1 2018, 10:06 AM

The change requires LDS bit in BUFFER_STORE_LDS_DWORD. This looks incorrect. LDS bit affects only MUBUF reads.

The change disallows TFE for BUFFER_STORE_LDS instructions as well as buffer loads to LDS. However, ISA manual disallows the latter case only: it says that it is illegal to set the instruction’s TFE bit for loads to LDS.

I believe that both LDS and TFE do not affect buffer stores. Perhaps it is worth to issue a warning or error in such cases. That's up to you.

This revision now requires changes to proceed.Mar 1 2018, 10:06 AM
dp added a comment.Mar 1 2018, 10:49 AM

AMD documentation explicitly states:

"MUBUF Instruction: BUFFER_STORE_LDS_DWORD
    LDS = 1
    IDXEN = 0
    OFFEN = 0
    ..."

See Gfx9_Shader_Programming, Section 3.9.8 "LDS_DMA : LDS to Memory buffer without VGPRs".

Regarding TFE, documentation says that TFE is valid for "fetch" instructions only. By design, TFE is useless with stores.
I believe this instruction is not a "fetch".

artem.tamazov accepted this revision.Mar 1 2018, 11:29 AM
In D43950#1024132, @dp wrote:

Regarding TFE, documentation says that TFE is valid for "fetch" instructions only. By design, TFE is useless with stores.
I believe this instruction is not a "fetch".

Sure. However, there is a difference between "invalid" and "useless". Let's decide which approach we like and use it consistently.
Right now TFE is not disabled in "conventional" buffer stores (I just checked that), but this patch disallows it in LDS stores.
This can be done in a separate patch, but let's not forget that.

In D43950#1024132, @dp wrote:
LDS = 1
IDXEN = 0
OFFEN = 0

Thanks. So we require LDS and disallow both idxen and offen.

LGTM provided that now we stay on the same page.

test/MC/AMDGPU/mubuf.s
775 ↗(On Diff #136544)

It seems strange that this is not "NOSICI: error: instruction not supported on this GPU"

This revision is now accepted and ready to land.Mar 1 2018, 11:29 AM
arsenm requested changes to this revision.Mar 2 2018, 12:06 PM
arsenm added inline comments.
lib/Target/AMDGPU/BUFInstructions.td
899 ↗(On Diff #136544)

This isn't a separate opcode according to the documentation? Isn't this just a modifier bit? i.e. this would be BUFFER_STORE_DWORD_lds and not have a separate string name either

This revision now requires changes to proceed.Mar 2 2018, 12:06 PM
dp added a comment.Mar 2 2018, 12:37 PM

According to AMD documentation, this is a separate opcode:

BUFFER_STORE_DWORD: opcode=28
BUFFER_STORE_LDS_DWORD: opcode=61

See e.g. "Vega" Instruction Set Architecture, page 175

dp added inline comments.Mar 2 2018, 2:35 PM
lib/Target/AMDGPU/BUFInstructions.td
899 ↗(On Diff #136544)

This is a separate opcode

dp marked an inline comment as done.Mar 5 2018, 11:08 AM

Dmitry, I believe you can go ahead.

lib/Target/AMDGPU/BUFInstructions.td
899 ↗(On Diff #136544)

Matt, if you are thinking that LDS attribute affects buffer stores, then this is not the case. There is a separate opcode for writing from LDS into memory.

arsenm accepted this revision.Mar 6 2018, 9:04 AM

LGTM

This revision is now accepted and ready to land.Mar 6 2018, 9:04 AM
This revision was automatically updated to reflect the committed changes.