The patch adds SPIRV-specific MC layer implementation, SPIRV object file support and SPIRVInstPrinter.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Iliya, since this is the 3rd patch of a total of 6, do I understand
correctly that the second one is https://reviews.llvm.org/D115786 from
Michal?
Same as the others, looks like a standard MC patch. I do have some inline comments, though.
llvm/include/llvm/MC/MCContext.h | ||
---|---|---|
71 | Is this really a new object file type? | |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCAsmInfo.cpp | ||
21 | A previous patch has a comment on data layout that SPIRV can be either little or big endian. Was that comment left-over from copy-paste or is this line just simplifying the implementation for now? If the former, removing the comment would be good. If the latter, adding a TODO here would be nice. |
llvm/include/llvm/MC/MCContext.h | ||
---|---|---|
71 | SPIR-V uses its own object file layout. A SPIR-V module is a single linear stream of words starting with some special words such as magic number, version number etc, followed by instruction stream. For more information, please refer to "2.3 Physical Layout of a SPIR-V Module and Instruction" of the SPIR-V specification. | |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCAsmInfo.cpp | ||
21 | Yes, it looks confusing. According to the specification, literals are always packed in little-endian manner, so I'll remove the comment. |
llvm/include/llvm/ADT/Triple.h | ||
---|---|---|
272 | A future new binary format will not need to touch the SPIRV line | |
llvm/include/llvm/MC/MCContext.h | ||
71 | below XCOFFAllocator (group with other binary formats) | |
llvm/include/llvm/MC/MCSPIRVObjectWriter.h | ||
20 | delete blank line | |
25 | delete unneeded dtor | |
llvm/include/llvm/MC/MCSection.h | ||
50 | ||
llvm/lib/MC/SPIRVObjectWriter.cpp | ||
48 | constexpr | |
67 | delete excess blank lines | |
71 | ||
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVInstPrinter.cpp | ||
40 | ||
106 | DFPImm is a double, not a float. | |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCCodeEmitter.cpp | ||
54 | remove llvm:: | |
127 | Add const whenever applicable. | does not need a pair of parens. |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCCodeEmitter.cpp | ||
---|---|---|
130 | ||
132 | ||
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCTargetDesc.cpp | ||
72 | If SyntaxVariant!=0 never makes sense, consider assert | |
114 | The code self explains. The comments should all be deleted. | |
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVTargetStreamer.cpp | ||
19 | Prefer {} for out-of-line definitions. |
Should include the customary Triple unit test changes
llvm/include/llvm/MC/MCSPIRVObjectWriter.h | ||
---|---|---|
40 | Don't need llvm:: inside llvm namespace |
Issues reported by Fangrui Song and Matt Arsenault were fixed. New triple unit tests were added.
Fix clang-format issues. I'm not sure that we need to apply this patch
- case Triple::SPIRV: return "spirv"; + case Triple::SPIRV: + return "spirv";
since it breaks the style of the surrounding code.
I have resolved the last clang-format issue by changing the code style for all Triples in getObjectFormatTypeName(). Also the updated patch fixes builds with shared libs, which Fangrui Song proposed to check.
This patch does not cause "x64 debian fail" (9 failed tests in libarcher :: races) since I see other builds that fail with the same issue.
llvm/include/llvm/ADT/Triple.h | ||
---|---|---|
272 | Sort the list alphabetically | |
llvm/include/llvm/MC/MCContext.h | ||
58 | Sort MCSectionSPIRV alphabetically | |
80–89 | Move IsSPIRV before IsWasm | |
137 | Sort | |
671 | Sort | |
llvm/include/llvm/MC/MCObjectFileInfo.h | ||
454 | Move before initWasmMCObjectFileInfo | |
llvm/include/llvm/MC/TargetRegistry.h | ||
988 | Move before Wasm | |
llvm/lib/MC/MCAsmBackend.cpp | ||
52 | Move before Wasm | |
llvm/lib/MC/MCObjectFileInfo.cpp | ||
1003 | Move before Wasm | |
1050 | Move before Wasm | |
1069 | Move before XCOFF | |
llvm/lib/MC/MCParser/AsmParser.cpp | ||
805 | Move before Wasm |
If you use a recent Clang to build llvm-project, there are several warnings:
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4859:11: warning: enumeration value 'SPIRV' not handled in switch [-Wswitch] llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2106:11: warning: enumeration value 'SPIRV' not handled in switch [-Wswitch] llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:6368:11: warning: enumeration value 'IsSPIRV' not handled in switch [-Wswitch] # I did not check whether this was due to a previous patch llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp:532:3: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
Does SPIRV need TargetLoweringObjectFileSPIRV? When GOFF was added in D106380, there was a test in test/CodeGen/SystemZ
This patch introduces a good chunk of MC stuff but there seems no accompanying test.
Thanks, I'll fix.
Does SPIRV need TargetLoweringObjectFileSPIRV?
I think not, since SPIRV does not use sections (other than text), relocations and other related things.
When GOFF was added in D106380, there was a test in test/CodeGen/SystemZ
This patch introduces a good chunk of MC stuff but there seems no accompanying test.
I think we cannot add a similar test in this patch for two reasons:
- GOFF was added to existing targets with implemented set of target passes. SPIRV target (in this 3rd patch) has not the passes yet and cannot generate asm and obj files.
- All content of SPIR-V modules in the form of text files are instructions. Thus, any rational test should contain SPIR-V instructions. AsmPrinter is implemented in the 4th patch, so theoretically we could add a simple test in the 4th patch (but it would require moving some code from the 5th/6th to the 4th patch of the series).
However, these MC changes are covered by the tests in the 6th patch, and all 6 patches will be committed to LLVM repository simultaneously, so I'm not sure what benefits do we really get by moving some tests from the 6th patch to the earlier ones.
Fixed issues mentioned by Fangrui Song.
This one will be fixed in the 5th patch:
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp:532:3: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVInstPrinter.cpp | ||
---|---|---|
87 | Remove assert and switch the dyn_casts above to cast |
@MaskRay as I understand the issues have been solved (see my previous replies and the updated patch). If yes, please approve this patch.
Apply s/unsigned int/unsigned/g to this patch and add dots at the end of sentences in comments as suggested Fangrui in the 4th patch.
A future new binary format will not need to touch the SPIRV line