This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][llvm-mc] Add support for sendmsg(...) syntax.
ClosedPublic

Authored by artem.tamazov on Apr 27 2016, 7:34 AM.

Details

Summary

Added support for sendmsg(MSG[, OP[, STREAM_ID]]) syntax
in s_sendmsg and s_sendmsghalt instructions.
The syntax matches the SP3 assembler/disassembler rules.
That is why implicit inputs (like M0 and EXEC) are not printed
to disassembly output anymore.

sendmsg(...) allows only _supported_ message types and attributes,
even if literals are used instead of symbolic names.
However, raw literal (without "sendmsg") still can be used,
and that allows for any 16-bit value.

Tests updated/added.

Diff Detail

Repository
rL LLVM

Event Timeline

artem.tamazov retitled this revision from to [AMDGPU][llvm-mc] Add support for sendmsg(...) syntax..
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.
artem.tamazov edited reviewers, added: SamWot; removed: sam.weinig.Apr 27 2016, 7:38 AM
artem.tamazov updated this object.
arsenm added inline comments.Apr 27 2016, 5:00 PM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1837–1839

cases at same indentation as switch

1883

There should be an enum for the symbolic name/values

1884

demorgans law this

1893

Capitalize and period

1901

Why is this loop needed?

lib/Target/AMDGPU/InstPrinter/AMDGPUInstPrinter.cpp
824

llvm_unreachable

832

llvm_unreachable

836

Single quotes

846

Space before (

851

llvm_unreachable

853

Single quotes

856

Another while(0)

SamWot added inline comments.Apr 28 2016, 3:54 AM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1838

This should be MatchOperand_NoMatch. Otherwise this will prevent other operand types to be parsed.

1845–1846

What are those error messages?
Is it "unrecognized instruction mnemonic"?

lib/Target/AMDGPU/SIInstructions.td
25–28

This and other Operands and AsmOperandClasses should be moved to SIInstrInfo.td. It might be done in different change.

artem.tamazov added inline comments.Apr 28 2016, 4:00 AM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1884

I expect extension of this expression to something like

if (! ((1 <= Operation.Id && Operation.Id <= 4)
    ||(IsSomeNewAmdHw() && 5 <= Operation.Id && Operation.Id <= 6))) {...

If you not insist, I would better keep current code to minimize future changes.

1901

Actually this is not a loop. "break" within do block; jumps just after while. That avoids gotos.

lib/Target/AMDGPU/InstPrinter/AMDGPUInstPrinter.cpp
856

Another idiom to avoid goto.

artem.tamazov added inline comments.Apr 28 2016, 4:02 AM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1838

Only immediates are supported in this context.

1845–1846

No. An immediate must fit into 16 bits.

artem.tamazov added inline comments.Apr 28 2016, 4:08 AM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1838

Pardon, immediates and sendmsg(...) handmade "type" are supported, but not other types. So we don't need to parse those.

SamWot added inline comments.Apr 28 2016, 5:12 AM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1838

LLVM parser tries to parse various operand types until it meets Success or ParseFail. If it meets NoMatch then it tryies next operand type. So if some instruction would have SendMsgImm and some other operands allowed in same place (for example if SendMsgImm is optional) then returning ParseFail will prevent trying to parse other operand.
For now this might work but I suggest to replace it with NoMatch here.

artem.tamazov marked an inline comment as done.Apr 29 2016, 4:38 AM
artem.tamazov added inline comments.
lib/Target/AMDGPU/SIInstructions.td
25–28

Will do separately.

artem.tamazov marked an inline comment as done.

Review-related fixes.

artem.tamazov marked 13 inline comments as done.Apr 29 2016, 5:03 AM
artem.tamazov marked an inline comment as done.Apr 29 2016, 5:06 AM
artem.tamazov added inline comments.
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1883

Matthew, please let me know if you want to rework hwreg(...) support in the same manner.

ParseFail -> NoMatch. Ready to next review round.

artem.tamazov marked 3 inline comments as done.Apr 29 2016, 5:34 AM
SamWot accepted this revision.May 4 2016, 6:22 AM
SamWot edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 4 2016, 6:22 AM
artem.tamazov requested a review of this revision.May 5 2016, 6:08 AM
artem.tamazov edited edge metadata.
artem.tamazov marked 2 inline comments as done.

(I am going to submit this soon if no objections.)

This revision was automatically updated to reflect the committed changes.