This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/SI: Fix encoding of flat instructions on VI
ClosedPublic

Authored by tstellarAMD on Dec 22 2015, 6:46 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

tstellarAMD retitled this revision from to AMDGPU/SI: Fix encoding of flat instructions on VI.
tstellarAMD updated this object.
tstellarAMD added a reviewer: arsenm.
tstellarAMD added a subscriber: llvm-commits.
arsenm added inline comments.Dec 22 2015, 6:51 PM
lib/Target/AMDGPU/CIInstructions.td
129 ↗(On Diff #43501)

missing spaces after ,

lib/Target/AMDGPU/SIInstrInfo.td
2598 ↗(On Diff #43501)

Why do these have different names? For TFE at least I'm pretty sure we will need separate opcodes for them, but I guess it can stay for now.

test/MC/AMDGPU/flat.s
4–8 ↗(On Diff #43501)

I noticed this before. Can you file a bug for this

Fix whitespace.

tstellarAMD marked an inline comment as done.Dec 23 2015, 7:11 AM
tstellarAMD added inline comments.
lib/Target/AMDGPU/SIInstrInfo.td
2598 ↗(On Diff #43534)

The assembler needs to know the full list of optional operands when parsing, so glc_flat, slc_flat, tfe_flat all map to the same list of optional operands. The optional operands are different for flat than mubuf, which is why they need a different operand type.

test/MC/AMDGPU/flat.s
4–9 ↗(On Diff #43534)

This is a bug in the AMDGPU backend and how we implement optional operands. I have an idea for generic optional operand handling which should fix this an simplify a number of things in the assembler.

arsenm accepted this revision.Dec 23 2015, 1:08 PM
arsenm edited edge metadata.

LGTM

lib/Target/AMDGPU/SIInstrInfo.td
2598 ↗(On Diff #43534)

I don't get why this would matter. We already have some instructions that may or may not have clamp etc.

This revision is now accepted and ready to land.Dec 23 2015, 1:08 PM
lib/Target/AMDGPU/SIInstrInfo.td
2598 ↗(On Diff #43534)

The difference is, for flat there are 3 possible optional args: glc,slc,tfe, but for flat atomic there are only two optional args: slc, tfe. so the operand sets need to have different names.

This revision was automatically updated to reflect the committed changes.