This is an archive of the discontinued LLVM Phabricator instance.

[SPIRV 3/6] Add MC layer, object file support and InstPrinter
ClosedPublic

Authored by iliya-diyachkov on Dec 31 2021, 2:38 PM.

Details

Summary

The patch adds SPIRV-specific MC layer implementation, SPIRV object file support and SPIRVInstPrinter.

Diff Detail

Event Timeline

iliya-diyachkov requested review of this revision.Dec 31 2021, 2:38 PM
Herald added a project: Restricted Project. · View Herald Transcript

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?

Alex, yes, you are right. Michal will correct his patch slightly later.

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.

MaskRay added inline comments.Jan 3 2022, 3:37 PM
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.

MaskRay added inline comments.Jan 3 2022, 3:37 PM
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.

iliya-diyachkov marked 20 inline comments as done.Jan 9 2022, 3:42 PM

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.

rengolin accepted this revision.Feb 2 2022, 3:41 AM

All comments were addressed and this looks good to go. Thanks!

This revision is now accepted and ready to land.Feb 2 2022, 3:41 AM
MaskRay added inline comments.Mar 11 2022, 7:51 PM
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

Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2022, 7:51 PM
MaskRay requested changes to this revision.EditedMar 11 2022, 8:13 PM

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.

This revision now requires changes to proceed.Mar 11 2022, 8:13 PM

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]

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]
iliya-diyachkov marked 12 inline comments as done.Mar 12 2022, 4:22 AM

Rebased. Fixed build.

arsenm added inline comments.Mar 14 2022, 2:38 PM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVInstPrinter.cpp
102

Single quotes

108

else if and use llvm_unreachable?

Fix the issue in SPIRVInstPrinter::printOperand noticed by Matt.

iliya-diyachkov marked 2 inline comments as done.Mar 14 2022, 4:29 PM
arsenm added inline comments.Mar 14 2022, 4:40 PM
llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVInstPrinter.cpp
87

Remove assert and switch the dyn_casts above to cast

The latest issue is fixed.

iliya-diyachkov marked an inline comment as done.Mar 15 2022, 4:21 AM
arsenm accepted this revision.Mar 15 2022, 6:56 AM

The patch was rebased.

@MaskRay, please approve the patch since the requested changes were done.

arsenm accepted this revision.Mar 21 2022, 5:52 AM

LGTM with a nit

llvm/lib/MC/MCSPIRVStreamer.cpp
24

Seems like a surprisingly big small size

llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCCodeEmitter.cpp
91

No Error:

The last issues found by Matt have been fixed.

iliya-diyachkov marked 2 inline comments as done.EditedMar 21 2022, 12:53 PM

LGTM with a nit

@arsenm thank you for the time and effort you spent on reviewing this project! I would be very grateful if you could take the next step in the review of the 4th, 5th and 6th patches, so we either would continue to fix the problems or commit the patches to the LLVM repository.

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.

@MaskRay as I understand the issues have been solved (see my previous replies and the updated patch). If yes, please approve this patch.

MaskRay accepted this revision.Mar 27 2022, 10:07 AM
This revision is now accepted and ready to land.Mar 27 2022, 10:07 AM

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.