Page MenuHomePhabricator

AMDGPU/SI: Assembler: Unify parsing/printing of operands.
ClosedPublic

Authored by nhaustov on Apr 27 2016, 3:34 AM.

Details

Summary

The goal is for each operand type to have its own parse function and
at the same time share common code for tracking state as different
instruction types share operand types (e.g. glc/glc_flat, etc).

Introduce parseAMDGPUOperand which can parse any optional operand.
DPP and Clamp/OMod have custom handling for now. Sam also suggested
to have class hierarchy for operand types instead of table. This
can be done in separate change.

Remove parseVOP3OptionalOps, parseDS*OptionalOps, parseFlatOptionalOps,
parseMubufOptionalOps, parseDPPOptionalOps.
Reduce number of definitions of AsmOperand's and MatchClasses' by using common base class.
Rename AsmMatcher/InstPrinter methods accordingly.
Print immediate type when printing parsed immediate operand.
Use 'off' if offset/index register is unused instead of skipping it to make it more readable (also agreed with SP3).
Update tests.

Diff Detail

Repository
rL LLVM

Event Timeline

nhaustov updated this revision to Diff 55180.Apr 27 2016, 3:34 AM
nhaustov retitled this revision from to AMDGPU/SI: Assembler: Unify parsing/printing of operands..
nhaustov updated this object.
nhaustov added a subscriber: llvm-commits.
SamWot edited edge metadata.Apr 27 2016, 5:16 AM

LGTM. Some minor comments.
This might be better to split this in several patches.

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
74–75 ↗(On Diff #55180)

Why do we need to rename Clamp and OMod?

550 ↗(On Diff #55180)

Consider using std::function instead of function pointer

1156–1157 ↗(On Diff #55180)

This better to be placed in ParseInstruction() method

1953 ↗(On Diff #55180)

Support for sdwa should be added here. It might be done later.

lib/Target/AMDGPU/SIInstrInfo.td
487–493 ↗(On Diff #55180)

This should be moved up before NamedOperandBit class.

500 ↗(On Diff #55180)

This should be moved up before NamedOperandBit class.

nhaustov updated this revision to Diff 55201.Apr 27 2016, 6:09 AM
nhaustov marked an inline comment as done.
nhaustov edited edge metadata.

Move sdwa_sel up and remove NamedBitMatchClass.
Move removal of prefix to ParseInstruction.

nhaustov marked 5 inline comments as done.Apr 27 2016, 6:11 AM

It's hard to split this change because operand changes depend on parsing changes and vice versa.

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
74–75 ↗(On Diff #55180)

Now print and parse function names correspond to name in .td file. It now conflicts with Clamp / printClamp for pre-SI clamp.

550 ↗(On Diff #55180)

This matches definition in OptionalOperand struct. It may actually become a virtual method in operand class.

1953 ↗(On Diff #55180)

Yes, I'll leave it for you.

SamWot accepted this revision.Apr 27 2016, 6:16 AM
SamWot edited edge metadata.

LGTM

This revision is now accepted and ready to land.Apr 27 2016, 6:16 AM
nhaustov marked 5 inline comments as done.Apr 27 2016, 6:37 AM
artem.tamazov accepted this revision.Apr 27 2016, 6:48 AM
artem.tamazov edited edge metadata.

Make it so.

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1971 ↗(On Diff #55201)

getLexer().is(AsmToken::Identifier) would be better.
getLexer().isNot(AsmToken::EndOfStatement) seems useless.

1984 ↗(On Diff #55201)

I would put an explanation why omod is handled separately.

nhaustov updated this revision to Diff 55210.Apr 27 2016, 6:55 AM
nhaustov edited edge metadata.

Use getLexer.is().
Add comment for omod.

nhaustov updated this revision to Diff 55251.Apr 27 2016, 9:50 AM

Merge with latest changes.

nhaustov updated this revision to Diff 55550.Apr 29 2016, 2:05 AM

Rebase on latest repository (with reverted TTMP quads changes).

This revision was automatically updated to reflect the committed changes.