[PATCH] D11671: WebAssembly: print basic integer assembly.

[Dan Gohman]

sunfish accepted this revision.
This revision is now accepted and ready to land.

================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:82
@@ +81,3 @@
+  unsigned NumDefs = MI->getDesc().getNumDefs();
+  assert(NumDefs <= 1);
+
----------------
&& "Instructions with multiple result values not implemented yet" or so.

================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:84
@@ +83,3 @@
+
+  if (NumDefs) {
+    const MachineOperand &MO = MI->getOperand(0);
----------------
Please use NumDefs != 0, for clarity.

================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:87
@@ +86,3 @@
+    unsigned Reg = MO.getReg();
+    assert(TargetRegisterInfo::isVirtualRegister(Reg));
+    OS << "(setlocal @" << TargetRegisterInfo::virtReg2Index(Reg) << ' ';
----------------
virtReg2Index asserts this, so it's not necessary to assert it here too.

================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:100
@@ +99,3 @@
+  case WebAssembly::RETURN:
+    OS << "return";
+    break;
----------------
Does the default case below not handle this?

================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:117
@@ +116,3 @@
+
+  if (NumDefs)
+    OS << ')';
----------------
!= 0, as above

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:149
@@ +148,3 @@
+  for (const SDValue Op : OutVals)
+    RetOps.push_back(Op);
+  const SDValue Ops[] = {Chain, OutVals.front()};
----------------
Instead of appending one element at a time, RetOps.append(OutVals.begin(), OutVals.end());

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:195
@@ +194,3 @@
+      fail(DL, DAG, "WebAssembly hasn't implemented non-i32 arguments");
+    if (In.ArgVT != MVT::i32)
+      fail(DL, DAG, "WebAssembly hasn't implemented non-i32 arguments");
----------------
It's not actually necessary to check the ArgVT here. If VT is MVT::i32, it doesn't matter what the original type was.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrCtrl.td:1
@@ +1,2 @@
+//===- WebAssemblyInstrCtrl.td-WebAssembly Call codegen support -*- tablegen -*-
+//
----------------
Update the top-line comment to match the file.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrCtrl.td:28
@@ +27,3 @@
+
+let Uses = [SP32,SP64], hasSideEffects = 1, isReturn = 1, isTerminator = 1,
+    hasCtrlDep = 1, isBarrier = 1 in {
----------------
Do we really need the Uses of SP32,SP64 here? It's a little unusual, since we only use one or the other for the stack pointer, so using both here will always mean that we're using a register that isn't actually live.

I see that ARM and AArch64 have return instruction use SP, but no other targets seem to do this. Perhaps we can omit these for now until we figure out whether they're really needed?

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrInfo.td:67
@@ -54,2 +66,3 @@
 include "WebAssemblyInstrCall.td"
+include "WebAssemblyInstrCtrl.td"
 include "WebAssemblyInstrInteger.td"
----------------
In-tree precedent for the filename here is X86InstrControl.td, so we should be WebAssemblyInstrControl.td.


http://reviews.llvm.org/D11671