This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][llvm-mc] Fixes to support buffer atomics.
ClosedPublic

Authored by artem.tamazov on May 13 2016, 1:56 PM.

Details

Summary

Fixes for MUBUF_Atomic instructions to make operand list valid:

  • For RTN insns, make a copy of $vdata_in operand as $vdata.
  • Do not add operand for GLC, it is hardcoded and comes as a token.

Workaround to avoid adding multiple optional default operands; more info in comments.
Tests added.

Diff Detail

Repository
rL LLVM

Event Timeline

artem.tamazov retitled this revision from to [AMDGPU][llvm-mc] Fixes to support buffer atomics..
artem.tamazov updated this object.
artem.tamazov set the repository for this revision to rL LLVM.
artem.tamazov added a project: Restricted Project.
artem.tamazov added a subscriber: Restricted Project.

Ping. BTW I am going to add some tests for buffer atomics.

artem.tamazov added inline comments.May 17 2016, 11:06 AM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1485–1487 ↗(On Diff #57249)

Previously added operand shall be examined here. Going to fix and update the patch soon.

artem.tamazov updated this object.

Don't add operand for GLC (add new cvtMubufAtomic() for that).
Fix w/a in parseIntWithPrefix().
Add ~1/3 of planned tests (more tomorrow).

artem.tamazov marked an inline comment as done.May 17 2016, 1:27 PM

Tests added, all done.

SamWot added inline comments.May 18 2016, 4:32 AM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
90–100 ↗(On Diff #57583)

I think there is no need in this define. This workaround is always enabled and I don't see when it could be disabled. When we will be able to get rid of this we will just remove this type of operands.

670–671 ↗(On Diff #57583)

Move this to private section of AMDGPUAsmParser where all private function reside. This really harms readability of code.

2142 ↗(On Diff #57583)

Do we need this check at all?
At least you can replace it with simple assert

SamWot added inline comments.May 18 2016, 4:46 AM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
2141 ↗(On Diff #57583)

You can create converter just for atomics with return value instead of converter for all atomics. In that case there would be no need in this check. This would be more stable because in change I'm currently working glc would be always parsed as immediate (not token) and this check will fail.

artem.tamazov added inline comments.May 18 2016, 5:23 AM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
90–100 ↗(On Diff #57583)

While there is no programmatic purpose of this macro (in this specific case), there is another duties for it.
It replaces comments like "// workaround for..." which I would insert otherwise.
It clearly denotes lines of code, so it is extremely easy to remove the workaround.
It provides programmatic link to the textual description of the workaround.
Will remove, if you insist ))

artem.tamazov marked 4 inline comments as done.
artem.tamazov added inline comments.
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
2123 ↗(On Diff #57599)

Good idea, thanks!

artem.tamazov marked 2 inline comments as done.May 18 2016, 5:26 AM
SamWot accepted this revision.May 18 2016, 5:33 AM
SamWot edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 18 2016, 5:33 AM
artem.tamazov requested a review of this revision.May 18 2016, 6:31 AM
artem.tamazov edited edge metadata.

I am going to commit this in 24hrs if no objections/comments arrive.

This revision was automatically updated to reflect the committed changes.