This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC] Enabled disassembler for image atomic operations
ClosedPublic

Authored by dp on Jan 17 2018, 9:00 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

dp created this revision.Jan 17 2018, 9:00 AM
arsenm added inline comments.Jan 17 2018, 9:08 AM
lib/Target/AMDGPU/MIMGInstructions.td
107 ↗(On Diff #130196)

What does the addr_rc.Size have to do with this?

dp added inline comments.Jan 17 2018, 9:21 AM
lib/Target/AMDGPU/MIMGInstructions.td
107 ↗(On Diff #130196)

We cannot disassemble all _V* variants of atomics. They have different address size, bit the size is not encoded.
In other words, all _V* variants are assembled to the same binary code.

So we have to pick one address size and the smallest size looks the safest.

BTW do you know why there are _V1, _V2 and _V4 variants but not V3? According to AMD docs, 3-element addresses are also valid.

dp added a comment.Jan 17 2018, 9:24 AM

Sorry, I used Size as if it were ValueType :-)

arsenm added inline comments.Jan 17 2018, 9:25 AM
lib/Target/AMDGPU/MIMGInstructions.td
107 ↗(On Diff #130196)

I think that needs a comment.

I don't think V3 is allowed for atomics based on the listed allowed dmask values

dp added inline comments.Jan 17 2018, 9:31 AM
lib/Target/AMDGPU/MIMGInstructions.td
107 ↗(On Diff #130196)

AFAIK dmask affects data size rather than address size.

dp updated this revision to Diff 130369.Jan 18 2018, 1:35 AM

Corrected after discussion with Matt:

  • code refactored for clarity;
  • added comments.
dp added a comment.Jan 18 2018, 6:45 AM

I started implementing 64-bit atomics and found out that disassembler should be enabled using 'MIMG_Mask' and 'channels'.
Otherwise we won't be able to reconstruct dst size.

I'm going to update this change one more time...

Dmitry, please consider the following (excerpt from GCN ISA manual):

It seems that data format and allowed DMASK values are implicitly defined by T#.
The number of address components also defined by T#.

Shall we invent a syntax elements to pass required surface information to the MIMG instructions?

dp added a comment.Jan 18 2018, 7:22 AM

This is an interesting idea, but T# is assumed to be in registers. Also note that GFX10 added a 'dim' field to encode address size.

I think we should fix known issues first and have at least 95% tests passed before contemplating any extensions/improvements.

In D42186#980293, @dp wrote:

This is an interesting idea, but T# is assumed to be in registers. Also note that GFX10 added a 'dim' field to encode address size.

The idea is to pass assumptions about T# to the assembler when MIMG instructions are being handled. Actual value of T# is obviously unavailable.

I think we should fix known issues first and have at least 95% tests passed before contemplating any extensions/improvements.

Absolutely. For now, let's make some (default) assumptions about T#. It would be nice (but not necessary) to see these assumptions implemented programmatically in this patch.

dp added a comment.Jan 23 2018, 8:44 AM

Looks like this is the final version. Initial implementation of 64-bit image atomics did not require any changes in this code.
Please review when you have time.

artem.tamazov accepted this revision.Jan 25 2018, 6:43 AM

LGTM.
BTW it would be nice to have a comment explaining the limitations of MIMG support. In short:

  • There is a fundamental limitation: only index of the beginning VGPR of address and data is specified/encoded. The number of actually used VGPRs are set implicitly (in the T#) and thus not known during assembling and disassembling.
  • This limitation can not be overcame unless violation of the SP3 compatibility requirement
  • Also there is a drawback related to CodeGen: the number of used VGPRs can be set to the power of 2 only, but actual number of VGPRs used is more diverse.
This revision is now accepted and ready to land.Jan 25 2018, 6:43 AM
This revision was automatically updated to reflect the committed changes.