Re: [PATCH] D11369: WebAssembly: basic bitcode → assembly CodeGen test

[Dan Gohman]

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

lgtm, with comments


================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:69
@@ +68,3 @@
+
+} // end of anonymous namespace
+
----------------
"end anonymous namespace" seems more popular in LLVM.

================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:79
@@ +78,3 @@
+  default:
+    MI->print(dbgs());
+    llvm_unreachable("Unhandled instruction");
----------------
This line should probably be in a DEBUG macro.

================
Comment at: lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp:39
@@ -38,3 +38,3 @@
 
-/// Return true if the specified function should have a dedicated frame pointer
-/// register.
+/// hasFP - Return true if the specified function should have a dedicated frame
+/// pointer register.
----------------
LLVM style now recommends against starting the comment with the function name now.

================
Comment at: lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp:45
@@ +44,3 @@
+      MF.getSubtarget().getRegisterInfo());
+  return (MFI->hasCalls() || MFI->hasVarSizedObjects() ||
+          MFI->isFrameAddressTaken() || MFI->hasStackMap() ||
----------------
Don't over-parenthesize return values.

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:21
@@ -20,2 +20,3 @@
 #include "WebAssemblyTargetObjectFile.h"
+
 #include "llvm/CodeGen/Analysis.h"
----------------
It doesn't seem common in LLVM to separate #includes with blank lines. Is there a reason for doing this?

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:103
@@ +102,3 @@
+  setStackPointerRegisterToSaveRestore(WebAssembly::SP32);
+  setStackPointerRegisterToSaveRestore(WebAssembly::SP64);
+  // Set up the register classes.
----------------
This should check the target and set *either* SP32 or SP64 as the stack pointer.

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:132
@@ +131,3 @@
+    const SmallVectorImpl<ISD::OutputArg> &Outs, LLVMContext &Context) const {
+  // FIXME: Implement using CallingConvention.td, similar to AArch64.
+  return Outs.size() <= 1;
----------------
It's actually likely that CallingConvention.td isn't appropriate. We don't have physical registers or stack arguments to deal with, which are the main things that file handles.

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:143
@@ +142,3 @@
+  if (Outs.size() > 1)
+    fail(DL, DAG, "WebAssembly can only return up to one value");
+
----------------
We can assert this, since CanLowerReturn only returns true if there's at most one output value, right?

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:157
@@ +156,3 @@
+  if (CallConv != CallingConv::C)
+    fail(DL, DAG, "WebAssembly doesn't support non-C calling conventions");
+  if (IsVarArg)
----------------
It'd be nice to have a corresponding check in LowerReturn too.

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:159
@@ +158,3 @@
+  if (IsVarArg)
+    fail(DL, DAG, "WebAssembly doesn't support varargs");
+  if (MF.getFunction()->hasStructRetAttr())
----------------
"yet". Even if we're planning to lower them in a different way, we should let people know that varargs are still planned to be supported :-).

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:161
@@ +160,3 @@
+  if (MF.getFunction()->hasStructRetAttr())
+    fail(DL, DAG, "WebAssembly doesn't support struct return");
+
----------------
"yet" here too, because I don't ultimately see any reason we shouldn't support this.


http://reviews.llvm.org/D11369