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

[Derek Schuff via llvm-commits]

dschuff added a comment.

I still want to add more testing but here's round 2 addressing the comments, and fixing the problem I mentioned with LowerVASTART above.


================
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 &
----------------
sunfish wrote:
> 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.
Turns out this was only relevant for a previous version of this change and isn't necessary after I fixed a bug; I put the assert back.

================
Comment at: lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp:51
@@ -50,3 +51,1 @@
-         MFI->hasStackMap() || MFI->hasPatchPoint() ||
-         RegInfo->needsStackRealignment(MF);
 }
----------------
sunfish wrote:
> Why did MFI->isFrameAddressTaken() go away here?
Because I added an assert on line 77 that the frame address is never taken. This change means that arguments are never passed on the stack, and even if there's a frame pointer or (I think?) stack realignment, the frame address still shouldn't be taken.  (Unless there's use of e.g. __builtin_frameaddress or something? but maybe we don't want to support that anyway?)

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:411
@@ +410,3 @@
+  }
+  // Add the pointer arg. its type is already
+  if (IsVarArg) {
----------------
jfb wrote:
> This looks like an unfinished
Moved the line with the rest of the output arg code and updated comment.

================
Comment at: test/CodeGen/WebAssembly/varargs.ll:141
@@ +140,3 @@
+; Store the second argument (the hidden vararg buffer pointer) into ap
+; CHEC K: i32.store $discard=, 0($0), $1
+  %1 = va_arg i8** %ap, i32
----------------
jfb wrote:
> ?
This was an attempt to tickle the bug I mentioned above, and wasn't meant to be checked in. Another test forthcoming.


http://reviews.llvm.org/D17048