[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/SPIRVCallLowering.cpp:31
+                                    Register SwiftErrorVReg) const {
+  assert(VRegs.size() < 2 && "All return types should use a single register");
+  if (Val) {
----------------
arsenm wrote:
> You can't guarantee this. A return of struct with multiple fields will hit it
In SPIR-V spec composite returns/formal args are allowed.  In the case of composites we need to keep the original type info and to avoid lowering of composite regs to multiple scalar regs. To achieve this we introduce a pass (it's not included in this series) before IR translation which clones such functions with composite returns/args to functions with scalar returns/args and keeps composite type info in metadata. So at the moment of CallLowering invocation we always expect VRegs size=1.
After this we restore the type info and associate it with the function type. Finally we get functions with composite returns/args at output. 
In the absence of this pass in the series we could temporary remove the asserts but I'm not sure if it's necessary change. Maybe it's better to add a comment about it?


================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:56
+    for (const auto &Arg : F.args()) {
+      assert(VRegs[i].size() == 1 && "Formal arg has multiple vregs");
+      ArgTypeVRegs.push_back(
----------------
arsenm wrote:
> This will happen for any struct arguments
Explained above.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:89
+                                  CallLoweringInfo &Info) const {
+  assert(Info.OrigRet.Regs.size() < 2 && "Call returns multiple vregs");
+
----------------
arsenm wrote:
> Struct returns
Explained above.


================
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;
+}
----------------
arsenm wrote:
> Why is there a fallback to 8? Why not just switch or check if 64?
In fact, we have one more target ("logical") and this 8 relates to it. However it is not included in this series, so I'll remove this fallback.


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


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

https://reviews.llvm.org/D116463