[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:
> iliya-diyachkov wrote:
> > 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?
> It's still valid IR, so you should do something better than assert. Also needs some test coverage
We have LIT tests with composite returns/args, but I'm afraid it's rather difficult to use them at the stage of this patch series, because the required functionality is not yet included and all the tests will fail.
Taking into account your advice, I assume that adding "// TODO..." before these asserts may be an appropriate solution for now.


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

https://reviews.llvm.org/D116463