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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.

I was on a trip for one week.



================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp:155
+                                      int *BytesRemoved) const {
+  llvm_unreachable("Branch removal not supported, as MBB info not propagated "
+                   "to OpPhi instructions. Try using -O0 instead.");
----------------
Can you verify this is unreachable? If it's unimplemented, you may call report_fatal_error


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp:175
+    ArrayRef<MachineOperand> Cond, const DebugLoc &DL, int *BytesAdded) const {
+  llvm_unreachable("Branch insertion not supported, as MBB info not propagated "
+                   "to OpPhi instructions. Try using -O0 instead.");
----------------
ditto


================
Comment at: llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp:36
+  // TODO: unify this with pointers legalization.
+  assert(Arch == Triple::spirv32 || Arch == Triple::spirv64);
+  return Arch == Triple::spirv32 ? 32 : 64;
----------------
The convention is to add `isSPIRV` (see isX86) and use it.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVSubtarget.h:69
+  }
+
+  const RegisterBankInfo *getRegBankInfo() const override {
----------------
I'd omit blank lines among these getter functions. aarch64/riscv use the compact style.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVTargetObjectFile.h:20
+public:
+  SPIRVTargetObjectFile() : TargetLoweringObjectFile() {}
+
----------------
Delete unneeded default ctor


================
Comment at: llvm/lib/Target/SPIRV/SPIRVTargetObjectFile.h:31
+                                   Align &Alignment) const override {
+    return TextSection;
+  }
----------------
The 3 functions need a comment. Using a non-data section seems weird to readers.


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

https://reviews.llvm.org/D116463