This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/SI: Add support for 32-bit immediate SMRD offsets on SI
ClosedPublic

Authored by tstellarAMD on Jul 29 2015, 11:27 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

tstellarAMD retitled this revision from to AMDGPU/SI: Add support for 32-bit immediate SMRD offsets on SI.
tstellarAMD updated this object.
tstellarAMD added a reviewer: arsenm.
tstellarAMD added a subscriber: llvm-commits.
arsenm added inline comments.Jul 29 2015, 11:50 AM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1581 ↗(On Diff #30933)

Typo: subtaget

1587–1589 ↗(On Diff #30933)

I think you need to be careful here if the immediate is == 255. It is still is uint<8>, but the encoding value will end up reading the next 4 bytes of instruction instead of the right value.

test/CodeGen/AMDGPU/smrd.ll
19 ↗(On Diff #30933)

These should probably be split into SI and CI lines. I think on SI this will end up using a 32-bit encoding and will have to use on a 64-bit one on CI for 255

test/MC/AMDGPU/smrd.s
11 ↗(On Diff #30933)

Should probably include a test with 0xff too

arsenm edited edge metadata.Jul 29 2015, 12:02 PM

Title / commit message should also say CI

tstellarAMD edited edge metadata.

Fixed typo and improve test for offset equal to 0xff.

tstellarAMD marked an inline comment as done.Jul 30 2015, 11:18 AM
tstellarAMD added inline comments.
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1587–1589 ↗(On Diff #31058)

Are you concerned that the value 0xff will be read as SRC_LITERAL rather than the actual offset? Because there is no way to specify SRC_LITERAL using the assembler.

test/CodeGen/AMDGPU/smrd.ll
19 ↗(On Diff #31058)

I double -checked and 32-bit immediates are only supported on CI. I updated this test to make sure it is using the 32-bit encoding of SMRD.

arsenm added inline comments.Jul 30 2015, 11:53 AM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1587–1589 ↗(On Diff #31058)

Yes. My understanding was you can't use 255 because that is the same as SRC_LITERAL and both are set in the same immediate field. The meaning of 255 would then change meaning depending on SI/CI.

test/CodeGen/AMDGPU/smrd.ll
19 ↗(On Diff #31058)

Because 255 is SQ_LITERAL on CI I think it will be 64-bit there

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1587–1589 ↗(On Diff #31058)

The meaning of 255 is controled by the IMM bit, so there shouldn't be an issue.
The encoding looks like this:

8-bit offset (0xff)
offset = 0xff
imm = 1

32-bit offset
offset = 0xff
imm = 0

arsenm added inline comments.Jul 30 2015, 12:15 PM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1587–1589 ↗(On Diff #31058)

OK, I didn't notice that bit. I think the assembler should probably just be smart about it and turn it on only if it sees an immediate that won't fit in 8-bits

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1587–1589 ↗(On Diff #31058)

The assembler already does this.

arsenm accepted this revision.Jul 30 2015, 12:49 PM
arsenm edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jul 30 2015, 12:49 PM
This revision was automatically updated to reflect the committed changes.