[WebAssembly] Change call_indirect stack semantics and printing order

[jgravelle-google]

Summary

For 0xc we need to change the order in which we consider the
arguments to call_indirect as being popped during register stackification.

[jgravelle-google]

This is the second option. We should go with either this or https://reviews.llvm.org/D25708 or something else entirely.

A third way that more directly expresses what I'd like is to be able to say in TableGen, "this instruction takes as input variable arguments, followed by a single fixed I32 argument", but that's a much more fundamental change that would probably only have a use case here.

[dschuff]

This ended up being worse than I expected. I think we should go the direction in D25708 also.

[jgravelle-google]

Abandoning in favor of https://reviews.llvm.org/D25708

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp

 
 void WebAssemblyInstPrinter::printRegName(raw_ostream &OS,
                                           unsigned RegNo) const {
   assert(RegNo != WebAssemblyFunctionInfo::UnusedReg);
   // Note that there's an implicit get_local/set_local here!
   OS << "$" << RegNo;
 }
 
+// TODO: Include this correctly
+bool IsCallIndirect(unsigned opcode);
+
 void WebAssemblyInstPrinter::printInst(const MCInst *MI, raw_ostream &OS,
                                        StringRef Annot,
                                        const MCSubtargetInfo & /*STI*/) {
   // Print the instruction (this uses the AsmStrings from the .td files).
   printInstruction(MI, OS);
 
   // Print any additional variadic operands.
   const MCInstrDesc &Desc = MII.get(MI->getOpcode());
-  if (Desc.isVariadic())
-    for (auto i = Desc.getNumOperands(), e = MI->getNumOperands(); i < e; ++i) {
+  if (Desc.isVariadic()) {
+    auto start = Desc.getNumOperands();
+    if (start > 0 && IsCallIndirect(MI->getOpcode())) {
+      --start;
+    }
+    for (auto i = start, end = MI->getNumOperands(); i < end; ++i) {
       if (i != 0)
         OS << ", ";
-      printOperand(MI, i, OS);
-    }
+      auto x = i;
+      if (IsCallIndirect(MI->getOpcode())) {
+        x = i + 1;
+        if (x >= end)
+          x = start;
+      }
+      printOperand(MI, x, OS);
+    }
 
   // Print any added annotation.
   printAnnotation(OS, Annot);
 
   if (CommentStream) {
     // Observe any effects on the control flow stack, for use in annotating
     // control flow label references.
     switch (MI->getOpcode()) {

lib/Target/WebAssembly/WebAssemblyInstrCall.td

 } // isCodeGenOnly = 1
 
 multiclass CALL<WebAssemblyRegClass vt, string prefix> {
   def CALL_#vt : I<(outs vt:$dst), (ins i32imm:$callee, variable_ops),
                    [(set vt:$dst, (WebAssemblycall1 (i32 imm:$callee)))],
                    !strconcat(prefix, "call\t$dst, $callee")>;
   def CALL_INDIRECT_#vt : I<(outs vt:$dst), (ins I32:$callee, variable_ops),
                             [(set vt:$dst, (WebAssemblycall1 I32:$callee))],
-                            !strconcat(prefix, "call_indirect\t$dst, $callee")>;
+                            !strconcat(prefix, "call_indirect\t$dst")>;
 }
 
 multiclass SIMD_CALL<ValueType vt, string prefix> {
   def CALL_#vt : SIMD_I<(outs V128:$dst), (ins i32imm:$callee, variable_ops),
                          [(set (vt V128:$dst),
                                (WebAssemblycall1 (i32 imm:$callee)))],
                          !strconcat(prefix, "call\t$dst, $callee")>;
   def CALL_INDIRECT_#vt : SIMD_I<(outs V128:$dst),
                                   (ins I32:$callee, variable_ops),
                                   [(set (vt V128:$dst),
                                         (WebAssemblycall1 I32:$callee))],
                                   !strconcat(prefix,
-                                             "call_indirect\t$dst, $callee")>;
+                                             "call_indirect\t$dst")>;
 }
 
 let Uses = [SP32, SP64], isCall = 1 in {
   defm : CALL<I32, "i32.">;
   defm : CALL<I64, "i64.">;
   defm : CALL<F32, "f32.">;
   defm : CALL<F64, "f64.">;
   defm : SIMD_CALL<v16i8, "i8x16.">;
   defm : SIMD_CALL<v8i16, "i16x8.">;
   defm : SIMD_CALL<v4i32, "i32x4.">;
   defm : SIMD_CALL<v4f32, "f32x4.">;
 
   def CALL_VOID : I<(outs), (ins i32imm:$callee, variable_ops),
                     [(WebAssemblycall0 (i32 imm:$callee))],
                     "call    \t$callee">;
   def CALL_INDIRECT_VOID : I<(outs), (ins I32:$callee, variable_ops),
                              [(WebAssemblycall0 I32:$callee)],
-                             "call_indirect\t$callee">;
+                             "call_indirect\t">;
 } // Uses = [SP32,SP64], isCall = 1
 
 } // Defs = [ARGUMENTS]
 
 // Patterns for matching a direct call to a global address.
 def : Pat<(i32 (WebAssemblycall1 (WebAssemblywrapper tglobaladdr:$callee))),
           (CALL_I32 tglobaladdr:$callee)>;
 def : Pat<(i64 (WebAssemblycall1 (WebAssemblywrapper tglobaladdr:$callee))),

lib/Target/WebAssembly/WebAssemblyRegStackify.cpp

 };
 } // end anonymous namespace
 
 char WebAssemblyRegStackify::ID = 0;
 FunctionPass *llvm::createWebAssemblyRegStackify() {
   return new WebAssemblyRegStackify();
 }
 
+// TODO: Find a better place for this to live
+bool IsCallIndirect(unsigned opcode) {
+  switch (opcode) {
+    case WebAssembly::CALL_INDIRECT_VOID:
+    case WebAssembly::CALL_INDIRECT_I32:
+    case WebAssembly::CALL_INDIRECT_I64:
+    case WebAssembly::CALL_INDIRECT_F32:
+    case WebAssembly::CALL_INDIRECT_F64:
+      return true;
+    default:
+      return false;
+  }
+}
+
 // Decorate the given instruction with implicit operands that enforce the
 // expression stack ordering constraints for an instruction which is on
 // the expression stack.
 static void ImposeStackOrdering(MachineInstr *MI) {
   // Write the opaque VALUE_STACK register.
   if (!MI->definesRegister(WebAssembly::VALUE_STACK))
     MI->addOperand(MachineOperand::CreateReg(WebAssembly::VALUE_STACK,
                                              /*isDef=*/true,

 /// A stack for walking the tree of instructions being built, visiting the
 /// MachineOperands in DFS order.
 class TreeWalkerState {
   typedef MachineInstr::mop_iterator mop_iterator;
   typedef std::reverse_iterator<mop_iterator> mop_reverse_iterator;
   typedef iterator_range<mop_reverse_iterator> RangeTy;
   SmallVector<RangeTy, 4> Worklist;
 
+  void addRange(const mop_iterator &Begin, const mop_iterator &End) {
+    if (Begin != End) {
+      const iterator_range<mop_iterator> Range(Begin, End);
+      Worklist.push_back(reverse(Range));
+    }
+  }
+
 public:
   explicit TreeWalkerState(MachineInstr *Insert) {
-    const iterator_range<mop_iterator> &Range = Insert->explicit_uses();
-    if (Range.begin() != Range.end())
-      Worklist.push_back(reverse(Range));
+    PushOperands(Insert);
   }
 
   bool Done() const { return Worklist.empty(); }
 
   MachineOperand &Pop() {
     RangeTy &Range = Worklist.back();
     MachineOperand &Op = *Range.begin();
     Range = drop_begin(Range, 1);
     if (Range.begin() == Range.end())
       Worklist.pop_back();
     assert((Worklist.empty() ||
             Worklist.back().begin() != Worklist.back().end()) &&
            "Empty ranges shouldn't remain in the worklist");
     return Op;
   }
 
   /// Push Instr's operands onto the stack to be visited.
   void PushOperands(MachineInstr *Instr) {
-    const iterator_range<mop_iterator> &Range(Instr->explicit_uses());
-    if (Range.begin() != Range.end())
-      Worklist.push_back(reverse(Range));
+    auto Uses = Instr->explicit_uses();
+    if (!IsCallIndirect(Instr->getOpcode())) {
+      addRange(Uses.begin(), Uses.end());
+    } else {
+      addRange(Uses.begin() + 1, Uses.end());
+      addRange(Uses.begin(), Uses.begin() + 1);
+    }
   }
 
   /// Some of Instr's operands are on the top of the stack; remove them and
   /// re-insert them starting from the beginning (because we've commuted them).
   void ResetTopOperands(MachineInstr *Instr) {
     assert(HasRemainingOperands(Instr) &&
            "Reseting operands should only be done when the instruction has "
            "an operand still on the stack");

 
 #ifndef NDEBUG
   // Verify that pushes and pops are performed in LIFO order.
   SmallVector<unsigned, 0> Stack;
   for (MachineBasicBlock &MBB : MF) {
     for (MachineInstr &MI : MBB) {
       if (MI.isDebugValue())
         continue;
-      for (MachineOperand &MO : reverse(MI.explicit_operands())) {
-        if (!MO.isReg())
+
+      // TODO: Refactor this to be cleaner
+      SmallVector<MachineOperand*, 4> operands;
+      for (auto &MO : MI.explicit_operands())
+        operands.push_back(&MO);
+      SmallVector<MachineOperand*, 4> actual_operands;
+      if (!IsCallIndirect(MI.getOpcode())) {
+        for (auto x : operands)
+          actual_operands.push_back(x);
+      } else {
+        MachineOperand* target = nullptr;
+        for (unsigned i = 0; i < operands.size(); ++i) {
+          auto op = operands[i];
+          if (op->isDef()) {
+            actual_operands.push_back(op);
             continue;
-        unsigned Reg = MO.getReg();
+          }
+          if (!target) {
+            target = op;
+            continue;
+          }
+          actual_operands.push_back(op);
+        }
+        if (target) {
+          actual_operands.push_back(target);
+        }
+      }
+
+      for (MachineOperand *MO : reverse(actual_operands)) {
+        if (!MO->isReg())
+          continue;
+        unsigned Reg = MO->getReg();
 
         if (MFI.isVRegStackified(Reg)) {
-          if (MO.isDef())
+          if (MO->isDef())
             Stack.push_back(Reg);
           else
             assert(Stack.pop_back_val() == Reg &&
                    "Register stack pop should be paired with a push");
         }
       }
     }
     // TODO: Generalize this code to support keeping values on the stack across
     // basic block boundaries.
     assert(Stack.empty() &&
            "Register stack pushes and pops should be balanced");
   }
 #endif
 
   return Changed;
 }

test/CodeGen/WebAssembly/call.ll

 ; CHECK-NEXT: .result i32{{$}}
 ; CHECK-NEXT: {{^}} i32.call_indirect $push[[NUM:[0-9]+]]=, $0{{$}}
 ; CHECK-NEXT: return $pop[[NUM]]{{$}}
 define i32 @call_indirect_i32(i32 ()* %callee) {
   %t = call i32 %callee()
   ret i32 %t
 }
 
+; CHECK-LABEL: call_indirect_arg:
+; CHECK-NEXT: .param i32, i32{{$}}
+; CHECK-NEXT: {{^}} call_indirect $1, $0{{$}}
+; CHECK-NEXT: return{{$}}
+define void @call_indirect_arg(void (i32)* %callee, i32 %arg) {
+  call void %callee(i32 %arg)
+  ret void
+}
+
+; CHECK-LABEL: call_indirect_arg_2:
+; CHECK-NEXT: .param i32, i32, i32{{$}}
+; CHECK-NEXT: {{^}} i32.call_indirect $drop=, $1, $2, $0{{$}}
+; CHECK-NEXT: return{{$}}
+define void @call_indirect_arg_2(i32 (i32, i32)* %callee, i32 %arg, i32 %arg2) {
+  call i32 %callee(i32 %arg, i32 %arg2)
+  ret void
+}
+
 ; CHECK-LABEL: tail_call_void_nullary:
 ; CHECK-NEXT: {{^}} call void_nullary@FUNCTION{{$}}
 ; CHECK-NEXT: return{{$}}
 define void @tail_call_void_nullary() {
   tail call void @void_nullary()
   ret void
 }