[PATCH] D116463: [SPIRV 4/6] Add target lowerling, TargetMachine and AsmPrinter

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVMCInstLower.h:18
+
+// SPIRVMCInstLower - This class is used to lower a MachineInstr into an MCInst.
+class LLVM_LIBRARY_VISIBILITY SPIRVMCInstLower {
----------------
https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments "Don’t duplicate function or class name at the beginning of the comment."


================
Comment at: llvm/lib/Target/SPIRV/SPIRVMCInstLower.h:21
+public:
+  SPIRVMCInstLower() {}
+  void Lower(const MachineInstr *MI, MCInst &OutMI) const;
----------------
delete unneeded ctor


================
Comment at: llvm/lib/Target/SPIRV/SPIRVRegisterBankInfo.h:34
+public:
+  SPIRVRegisterBankInfo() = default;
+
----------------
delete unneeded ctor. ditto in many places


================
Comment at: llvm/lib/Target/SPIRV/SPIRVRegisterInfo.cpp:30
+// Dummy to not crash RegisterClassInfo.
+static const MCPhysReg CalleeSavedReg = SPIRV::NoRegister;
+
----------------
Use `CSR_NoRegs_SaveList` instead


================
Comment at: llvm/lib/Target/SPIRV/SPIRVRegisterInfo.h:24
+struct SPIRVRegisterInfo : public SPIRVGenRegisterInfo {
+
+  SPIRVRegisterInfo();
----------------
delete unneeded blank line. ditto in many places


================
Comment at: llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp:63
+                                                          StringRef FS) {
+  ParseSubtargetFeatures(CPU, /* TuneCPU */ CPU, FS);
+
----------------
The canonical form is `/*TuneCPU=*/CPU` accepted by both clang-format and clang-tidy.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp:87
+bool SPIRVSubtarget::canDirectlyComparePointers() const {
+  bool Res = isAtLeastVer(TargetSPIRVVersion, 14);
+  return Res;
----------------
remove used-once variable


================
Comment at: llvm/lib/Target/SPIRV/SPIRVSubtarget.h:59
+
+  // TODO Some of these fields might work without unique_ptr.
+  //      But they are shared with other classes, so if the SPIRVSubtarget
----------------
This should be fixed


================
Comment at: llvm/lib/Target/SPIRV/SPIRVSubtarget.h:106
+
+  bool isLittleEndian() const { return true; }
+
----------------
delete unneeded function.


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

https://reviews.llvm.org/D116463