[PATCH] D17048: [WebAssembly] Switch varags calling convention to use a register

[Dan Gohman via llvm-commits]

sunfish added inline comments.

================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp:149
@@ -148,3 +148,1 @@
   } else if (Op.isImm()) {
-    assert((OpNo < MII.get(MI->getOpcode()).getNumOperands() ||
-            (MII.get(MI->getOpcode()).TSFlags &
----------------
dschuff wrote:
> I don't really understand what this assert is for. Is it OK to remove it? Should I put in something else?
There are two categories of instructions with variadic arguments: calls and tableswitch. Call arguments are all registers from LLVM's perspective (if they are constants, they get loaded into "registers" with `i32.const` etc.). Tableswitch arguments are all immediates. We use the WebAssemblyII::VariableOpIsImmediate flag to indicate to naive code which kind of operand to expect in the variadic portion of an instruction. This assert is just asserting that if that flag is set, and we're in the variadic portion, that we are actually seeing an immediate. This assert is relevant, so we shouldn't remove it.

================
Comment at: lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp:51
@@ -50,3 +51,1 @@
-         MFI->hasStackMap() || MFI->hasPatchPoint() ||
-         RegInfo->needsStackRealignment(MF);
 }
----------------
Why did MFI->isFrameAddressTaken() go away here?

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:621
@@ +620,3 @@
+
+  // XXX if ARGUMENTS is not live-in to the block already this doesn't make it
+  // live in.
----------------
dschuff wrote:
> One issue I currently have is that if ARGUMENTS is not live-in to the current block, then I get  machineinstr verifier error because adding this reference does not add ARGUMENTS to the live-ins for the block. I could add it manually here, but I don't see a good way to get the current MBB from the current SelectionDAG, so maybe that's not the right thing to do. Any ideas?
In the entry block, when we're creating the ARGUMENTs for the fixed arguments, we should create the ARGUMENT for the vararg pointer too, and remember which virtual register we put it in. LowerVASTART should then be able to just read this register, without needing ARGUMENTS live to wherever it happens to be.


http://reviews.llvm.org/D17048