[PATCH] D116463: [SPIRV 4/6] Add target lowerling, TargetMachine and AsmPrinter
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 3 15:46:26 PST 2022
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
More information about the llvm-commits
mailing list