Hwreg(...) syntax implementation unified with sendmsg(...).
Common strings moved to Utils
MathExtras.h functionality utilized.
Added missing build dependency in Disassembler.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp | ||
---|---|---|
1727–1735 | Why do you need to change this? Previous variant looks much more readable for me? | |
1801 | You should return MatchOperand_ParseFail here.
| |
lib/Target/AMDGPU/InstPrinter/AMDGPUInstPrinter.cpp | ||
906–920 | Why do you need to change this? Previous variant looks much more readable for me? |
BTW thanks for reviewing))
lib/Target/AMDGPU/InstPrinter/AMDGPUInstPrinter.cpp | ||
---|---|---|
28–32 | That is planned to be done in separate change. For now, there is | |
test/MC/AMDGPU/sopp-err.s | ||
39 | Actually, there is a caret symbol which points to the operand: C:\work-1\asm-git-1\playground\debugging.s:1:28: error: not a valid operand. s_sendmsg sendmsg(2, 3, 0, 0) ^ But normally we do not check everything. Shall we? |
lib/Target/AMDGPU/InstPrinter/AMDGPUInstPrinter.cpp | ||
---|---|---|
28–32 | Pls let me know if you think it is better to do that just in this patch. |
lib/Target/AMDGPU/InstPrinter/AMDGPUInstPrinter.cpp | ||
---|---|---|
28–32 | Probably best to just do it now |
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp | ||
---|---|---|
1719 | Thanks for the tip. It seems that isShiftedUInt() can be also used in some places... Will try to use as much as possible stuff from MathExtras.h and update the review. Note that current design (factored out enums and strings) does not look readable and easy to use. Perhaps it is worth to implement and use deeper abstractions, perhaps classes which clearly encapsulate sendmsg/hwreg functionality required for the parser/printer. For example, Hwreg::getIdSymbolic(enum Id id) or HwReg::getIdNumeric(StringRef& identifier) etc. Let's do that in the future, when time permits. |
LGTM
lib/Target/AMDGPU/Utils/AMDGPUAsmParserPrinter.h | ||
---|---|---|
2 | Can you rename this file to something like AMDGPUAsmUtils? This name confuses me alot: there are AsmParser, AsmPrinter and InstPrinter. |
Can you use isMask_32 for this?