This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] [llvm-mc] [VI] Fix encoding of LDS/GDS instructions.
ClosedPublic

Authored by artem.tamazov on Feb 15 2016, 10:30 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

artem.tamazov retitled this revision from to [AMDGPU] [llvm-mc] [VI] Fix encoding of LDS/GDS instructions..
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.Feb 15 2016, 10:31 AM
artem.tamazov edited subscribers, added: SamWot, vpykhtin; removed: arsenm.Feb 15 2016, 10:42 AM
arsenm accepted this revision.Feb 15 2016, 11:29 AM
arsenm edited edge metadata.

LGTM

This revision is now accepted and ready to land.Feb 15 2016, 11:29 AM
tstellarAMD requested changes to this revision.Feb 16 2016, 6:40 AM
tstellarAMD edited edge metadata.
tstellarAMD added inline comments.
test/MC/AMDGPU/ds.s
10 ↗(On Diff #48000)

Why are there now VI checks here and in most of this test file?

This revision now requires changes to proceed.Feb 16 2016, 6:40 AM
test/MC/AMDGPU/ds.s
10 ↗(On Diff #48000)

Typo: I meant: Why are there *no* VI checks ...

artem.tamazov added inline comments.Feb 17 2016, 7:53 AM
test/MC/AMDGPU/ds.s
10 ↗(On Diff #48000)

Literally answering, because the creator of that file didn't add those.

This patch is intended to fix two specific cases (ds_write_b32 and ds_read2_b32), so I didn't bothered myself to add more VI test cases. As far as I see now, you want to have those, so I will do and update the patch.

tstellarAMD accepted this revision.Feb 17 2016, 8:04 AM
tstellarAMD edited edge metadata.

Ok, that's fine then. Based on the commit summary I thought you had fixed the encoding for all DS instruction and not just a few. I don't mind if you add the VI tests in a follow on patch. I'll commit this one.

This revision is now accepted and ready to land.Feb 17 2016, 8:04 AM

Ok, that's fine then. Based on the commit summary I thought you had fixed the encoding for all DS instruction and not just a few. I don't mind if you add the VI tests in a follow on patch. I'll commit this one.

Too late, I already started the work ;)

Ok, that's fine then. Based on the commit summary I thought you had fixed the encoding for all DS instruction and not just a few. I don't mind if you add the VI tests in a follow on patch. I'll commit this one.

BTW it really fixes all DS insns, but I had only two proven encodings on hand. The testcases I am going to add just register existing implementation (encoding looks valid but not thoroughly checked).

artem.tamazov edited edge metadata.

Added: Tests of DS encoding for VI target.

artem.tamazov marked 3 inline comments as done.Feb 17 2016, 9:27 AM
This revision was automatically updated to reflect the committed changes.