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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVRegisterInfo.cpp:30
+// Dummy to not crash RegisterClassInfo.
+static const MCPhysReg CalleeSavedReg = SPIRV::NoRegister;
+
----------------
iliya-diyachkov wrote:
> 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?
It's the same thing, not much difference. It's not worth defining SPIRCallingConv.td just for this


================
Comment at: llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp:35-36
+  const auto Arch = TT.getArch();
+  // TODO: unify this with pointers legalization
+  return Arch == Triple::spirv32 ? 32 : Arch == Triple::spirv64 ? 64 : 8;
+}
----------------
Why is there a fallback to 8? Why not just switch or check if 64?


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


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

https://reviews.llvm.org/D116463