[WebAssembly] Split and recombine multivalue calls for ISel

[tlively]

Summary

Multivalue calls both take and return an arbitrary number of
arguments, but ISel only supports one or the other in a single
instruction. To get around this, calls are modeled as two pseudo
instructions during ISel. These pseudo instructions, CALL_PARAMS and
CALL_RESULTS, are recombined into a single CALL MachineInstr in a
custom emit hook.

RegStackification and the MC layer will additionally need to be made
aware of multivalue calls before the tests will produce correct
output.

[merge_guards_bot]

clang-format: pass.

Build artifacts: console-log.txt, diff.json

[tlively]

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

469

This llvm_unreachable truly is unreachable because it comes after the return. There is a default case at the top of the switch that contains another llvm_unreachable, so this one was both incorrect and redundant.

[aheejin]

Nice! Are there no tests because we don't have the assembly printer support yet?

llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp

223

Nit: Does this i32 have a meaning? If not, can we use a chain (MVT::Other) instead?

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

440

Is CallParams guaranteed to be the previous node? What we did in ISelDAGToDAG is to make CallParams an operand of CallResult. While they have data dependences and CallResult will be guaranteed to be selected after CallParams, I'm not sure if there can't be any other instructions in between. Can't we get the operand of CallResults?

451

How do we assign which is defs and which is uses in this new CALL instruction?

llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td

59

CALL->CALL_PARAMS?

63

Sorry for real nit: Can we make comments wrap to 80 cols..? Lines kind of look to short.. I think there should be a shortcut in most editors for doing that.

70

Isn't this also isPseudo = 1?

[tlively]

• Address simple comments

[merge_guards_bot]

Unit tests: pass. 60863 tests passed, 0 failed and 726 were skipped.

clang-format: pass.

Build artifacts: diff.json, CMakeCache.txt, console-log.txt, test-results.xml

[tlively]

llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp

223

There's no meaning, but I have not been able avoid assertion failures using MTV::Other. I'll investigate further without spending too much time on it.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

440

Yeah, good point. I didn't see a simple API for getting defs of operands from MachineInstrs, but making this more robust is probably worth some extra code.

451

MachineOperands store this information on themselves and we follow the convention here of having the defs before the uses so things like getNumDefs should still work. This will be a bigger problem at the MC layer, where the def and use information is erased.

[tlively]

• Use MVT::Other for link, rebase

[merge_guards_bot]

Unit tests: pass. 60863 tests passed, 0 failed and 726 were skipped.

clang-format: pass.

Build artifacts: diff.json, CMakeCache.txt, console-log.txt, test-results.xml

[tlively]

llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp

223

Ok, I've switched this to use MVT::Other. This just required an update to the previous CL to avoid a nullptr dereference.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

440

Now that I switched to using MVT::Other, there isn't even a shared register to look for. I think we have no choice but to blindly hope that these instructions are emitted next to each other :/

[aheejin]

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

440

Oh, if changing to MTV::Other makes things harder, just don't do it ;( Sorry I was just suggesting.
Oh the other hand, can we use the MTV::Glue thing? I haven't used it myself and I'm not super familiar with it, but the definition of that is to glue nodes together, right? If we can actually guarantee they are glued together that's gonna be good.

[tlively]

• Switch to using MVT::Glue

[tlively]

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

440

MVT::Glue works! There must have been something else wrong when I experimented with it before.

[merge_guards_bot]

Unit tests: fail. 61975 tests passed, 1 failed and 783 were skipped.

failed: LLVM.Bindings/Go/go.test

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[aheejin]

LGTM. Please rebase this onto D71484; this currently contains diff of that CL too.

llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp

case ISD::INTRINSIC_W_CHAIN: {
     }
     }
     break;
   }
   case WebAssemblyISD::CALL: {
     // CALL has both variable operands and variable results, but ISel only
     // supports one or the other. Split calls into two nodes glued together, one
     // for the operands and one for the results. These two nodes will be
-    // recombined in a custom inserter hook.
-    // TODO: Split CALL
+    // recombined in a custom inserter hook into a single MachineInstr.
     SmallVector<SDValue, 16> Ops;
     for (size_t i = 1; i < Node->getNumOperands(); ++i) {
       SDValue Op = Node->getOperand(i);
       if (Op->getOpcode() == WebAssemblyISD::Wrapper)
         Op = Op->getOperand(0);
       Ops.push_back(Op);
     }
     Ops.push_back(Node->getOperand(0));
-    MachineSDNode *Call =
-        CurDAG->getMachineNode(WebAssembly::CALL, DL, Node->getVTList(), Ops);
-    ReplaceNode(Node, Call);
+    MachineSDNode *CallParams =
+        CurDAG->getMachineNode(WebAssembly::CALL_PARAMS, DL, MVT::Glue, Ops);
+    SDValue Link(CallParams, 0);
+    MachineSDNode *CallResults = CurDAG->getMachineNode(
+        WebAssembly::CALL_RESULTS, DL, Node->getVTList(), Link);
+    ReplaceNode(Node, CallResults);
     return;
   }
 
   default:
     break;
   }
 
   // Select the default instruction.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

BuildMI(*DoneMBB, DoneMBB->begin(), DL, TII.get(TargetOpcode::PHI), OutReg)
       .addReg(FalseReg)
       .addMBB(FalseMBB)
       .addReg(TrueReg)
       .addMBB(TrueMBB);
 
   return DoneMBB;
 }
 
+static MachineBasicBlock *LowerCallResults(MachineInstr &CallResults,
+                                           DebugLoc DL, MachineBasicBlock *BB,
+                                           const TargetInstrInfo &TII) {
+  MachineInstr &CallParams = *CallResults.getPrevNode();
+  assert(CallParams.getOpcode() == WebAssembly::CALL_PARAMS);
+  assert(CallResults.getOpcode() == WebAssembly::CALL_RESULTS);
+
+  MachineFunction &MF = *BB->getParent();
+  const MCInstrDesc &MCID = TII.get(WebAssembly::CALL);
+  MachineInstrBuilder MIB(MF, MF.CreateMachineInstr(MCID, DL));
+
+  for (auto Def : CallResults.defs())
+    MIB.add(Def);
+  for (auto Use : CallParams.uses())
+    MIB.add(Use);
+
+  BB->insert(CallResults.getIterator(), MIB);
+  CallParams.eraseFromParent();
+  CallResults.eraseFromParent();
+
+  return BB;
+}
+
 MachineBasicBlock *WebAssemblyTargetLowering::EmitInstrWithCustomInserter(
     MachineInstr &MI, MachineBasicBlock *BB) const {
   const TargetInstrInfo &TII = *Subtarget->getInstrInfo();
   DebugLoc DL = MI.getDebugLoc();
 
   switch (MI.getOpcode()) {
   default:
     llvm_unreachable("Unexpected instr type to insert");
case WebAssembly::FP_TO_UINT_I32_F64:
     return LowerFPToInt(MI, DL, BB, TII, true, false, true,
                         WebAssembly::I32_TRUNC_U_F64);
   case WebAssembly::FP_TO_SINT_I64_F64:
     return LowerFPToInt(MI, DL, BB, TII, false, true, true,
                         WebAssembly::I64_TRUNC_S_F64);
   case WebAssembly::FP_TO_UINT_I64_F64:
     return LowerFPToInt(MI, DL, BB, TII, true, true, true,
                         WebAssembly::I64_TRUNC_U_F64);
-    llvm_unreachable("Unexpected instruction to emit with custom inserter");
+  case WebAssembly::CALL_RESULTS:
+    return LowerCallResults(MI, DL, BB, TII);
   }
 }
 
 const char *
 WebAssemblyTargetLowering::getTargetNodeName(unsigned Opcode) const {
   switch (static_cast<WebAssemblyISD::NodeType>(Opcode)) {
   case WebAssemblyISD::FIRST_NUMBER:
   case WebAssemblyISD::FIRST_MEM_OPCODE:

llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td

I<(outs rt:$dst),
       (outs), (ins TypeIndex:$type, i32imm:$flags),
       [],
       !strconcat(prefix, "call_indirect\t$dst"),
       !strconcat(prefix, "call_indirect\t$type"),
       0x11>,
     Requires<preds>;
 }
 
+// CALL should take both variadic arguments and produce variadic results, but
+// this is not possible to model directly. Instead, we select calls to a
+// CALL_PARAMS taking variadic aguments linked with a CALL_RESULTS that handles
+// producing the call's variadic results. We recombine the two in a custom
+// inserter hook after DAG ISel, so passes over MachineInstrs will only ever
+// observe CALL nodes with all of the expected variadic uses and defs.
+let isPseudo = 1 in
+defm CALL_PARAMS :
+  I<(outs), (ins function32_op:$callee, variable_ops),
+    (outs), (ins function32_op:$callee), [],
+    "call_params\t$callee", "call_params\t$callee", -1>;
+
+let variadicOpsAreDefs = 1, usesCustomInserter = 1, isPseudo = 1 in
+defm CALL_RESULTS :
+  I<(outs), (ins variable_ops), (outs), (ins), [],
+     "call_results", "call_results", -1>;
+
 let Uses = [SP32, SP64], isCall = 1 in {
 
-// TODO: Split CALL into separate nodes for operands and results.
 // TODO: Add an indirect version of the variadic call, delete CALL_*
-let variadicOpsAreDefs = 1 in
 defm CALL :
   I<(outs), (ins function32_op:$callee, variable_ops),
     (outs), (ins function32_op:$callee), [],
-    "call    \t$callee", "call\t$callee", 0x10>;
+    "call\t$callee", "call\t$callee", 0x10>;
 
 defm "" : CALL<i32, I32, "i32.">;
 defm "" : CALL<i64, I64, "i64.">;
 defm "" : CALL<f32, F32, "f32.">;
 defm "" : CALL<f64, F64, "f64.">;
 defm "" : CALL<exnref, EXNREF, "exnref.", [HasExceptionHandling]>;
 defm "" : CALL<v16i8, V128, "v128.", [HasSIMD128]>;
 defm "" : CALL<v8i16, V128, "v128.", [HasSIMD128]>;