[PATCH] D116462: [SPIRV 3/6] Add MC layer, object file support and InstPrinter

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/ADT/Triple.h:244
     XCOFF,
+    SPIRV
   };
----------------
A future new binary format will not need to touch the SPIRV line


================
Comment at: llvm/include/llvm/MC/MCContext.h:124
     SpecificBumpPtrAllocator<MCInst> MCInstAllocator;
+    SpecificBumpPtrAllocator<MCSectionSPIRV> SPIRVAllocator;
 
----------------
below XCOFFAllocator (group with other binary formats)


================
Comment at: llvm/include/llvm/MC/MCSPIRVObjectWriter.h:19
+class MCSPIRVObjectTargetWriter : public MCObjectTargetWriter {
+
+protected:
----------------
delete blank line


================
Comment at: llvm/include/llvm/MC/MCSPIRVObjectWriter.h:24
+public:
+  ~MCSPIRVObjectTargetWriter() = default;
+
----------------
delete unneeded dtor


================
Comment at: llvm/include/llvm/MC/MCSection.h:50
+    SV_XCOFF,
+    SV_SPIRV
   };
----------------



================
Comment at: llvm/lib/MC/SPIRVObjectWriter.cpp:47
+  // does) requires some refactoring of MCAssembler::VersionInfoType.
+  uint32_t Major = 1;
+  uint32_t Minor = 0;
----------------
constexpr


================
Comment at: llvm/lib/MC/SPIRVObjectWriter.cpp:66
+  uint64_t StartOffset = W.OS.tell();
+
+  writeHeader(Asm);
----------------
delete excess blank lines


================
Comment at: llvm/lib/MC/SPIRVObjectWriter.cpp:70
+  for (const MCSection &S : Asm) {
+    Asm.writeSectionData(W.OS, &S, Layout);
+  }
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVInstPrinter.cpp:39
+      if (!SkipFirstSpace || i != StartIndex) {
+        O << ' ';
+      }
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVInstPrinter.cpp:105
+    } else if (Op.isDFPImm()) {
+      O << formatImm((float)Op.getDFPImm());
+    } else {
----------------
DFPImm is a double, not a float.



================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCCodeEmitter.cpp:53
+private:
+  llvm::FeatureBitset computeAvailableFeatures(const FeatureBitset &FB) const;
+  void verifyInstructionPredicates(
----------------
remove `llvm::`


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCCodeEmitter.cpp:126
+  uint32_t NumWords = MI.getNumOperands() + 1;
+  uint32_t FirstWord = ((NumWords << 16) | OpCode);
+  OSE.write<uint32_t>(FirstWord);
----------------
Add `const` whenever applicable.

`|` does not need a pair of parens.


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCCodeEmitter.cpp:129
+
+  // Emit the instruction arguments (emitting the output type first if present)
+  if (hasType(MI, MCII, MRI)) {
----------------



================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCCodeEmitter.cpp:131
+  if (hasType(MI, MCII, MRI)) {
+    emitTypedInstrOperands(MI, OSE);
+  } else {
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCTargetDesc.cpp:71
+                                               const MCRegisterInfo &MRI) {
+  if (SyntaxVariant == 0)
+    return new SPIRVInstPrinter(MAI, MII, MRI);
----------------
If `SyntaxVariant!=0` never makes sense, consider assert


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCTargetDesc.cpp:113
+
+    // Register the MC code emitter
+    TargetRegistry::RegisterMCCodeEmitter(*T, createSPIRVMCCodeEmitter);
----------------
The code self explains. The comments should all be deleted.


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVTargetStreamer.cpp:18
+SPIRVTargetStreamer::SPIRVTargetStreamer(MCStreamer &S) : MCTargetStreamer(S) {}
+SPIRVTargetStreamer::~SPIRVTargetStreamer() = default;
----------------
Prefer `{}` for out-of-line definitions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116462/new/

https://reviews.llvm.org/D116462