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

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVRegisterInfo.cpp:30
+// Dummy to not crash RegisterClassInfo.
+static const MCPhysReg CalleeSavedReg = SPIRV::NoRegister;
+
----------------
MaskRay wrote:
> Use `CSR_NoRegs_SaveList` instead
It looks like this was borrowed from AMDGPU (see R600RegisterInfo::getCalleeSavedRegs in R600RegisterInfo.cpp).
SPIRV target does not use physical registers and does not define SPIRVCallingConv.td. Several targets (like NVPTX, WebAssembly) do not use *CallingConv.td and just have 
```
static const MCPhysReg CalleeSavedRegs[] = { 0 }
```
Why wouldn't SPIRV use the same?


================
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
----------------
MaskRay wrote:
> This should be fixed
Sorry, this comment doesn't apply to these pointers. They are standard GlobalISel related APIs. I'll correct the comment.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVTargetMachine.h:46
+  bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const override {
+    return true;
+  }
----------------
arsenm wrote:
> The select implementation disagrees?
Yes, it's an error.


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

https://reviews.llvm.org/D116463