[PATCH] D12219: WebAssembly: Implement call

[Dan Gohman via llvm-commits]

sunfish added a comment.

Not done reviewing, but here are some comments so far.


================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:136
@@ +135,3 @@
+    case MachineOperand::MO_GlobalAddress: {
+      OS << " `" << MO.getGlobal()->getName();
+    } break;
----------------
Is this a mismatched quote?

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:216
@@ +215,3 @@
+    Chain = Res.getValue(1);
+  }
+
----------------
This is likely why the call_void is getting deleted. It's necessary to hook up the Chain result even when there is no return value.

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:321
@@ +320,3 @@
+  if (GA->getOffset() != 0)
+    fail(DL, DAG, "WebAssembly expects no offsets on global addresses");
+  if (GA->getTargetFlags() != 0)
----------------
We should implement isOffsetFoldingLegal() to always return false, and then we can change this to assert that the offset is 0.

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:323
@@ +322,3 @@
+  if (GA->getTargetFlags() != 0)
+    fail(DL, DAG, "WebAssembly doesn't expect flags on global addresses");
+  if (GA->getAddressSpace() != 0)
----------------
TargetFlags will only get set if the target sets them, and since we don't set them, this won't happen, so we can assert this too.

================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.h:30
@@ -30,2 +29,3 @@
+#undef HANDLE_NODETYPE
   // add memory opcodes starting at ISD::FIRST_TARGET_MEMORY_OPCODE here...
 };
----------------
Given the preprocessor magic, this comment should move to WebAssemblyISD.def.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrCall.td:22
@@ +21,3 @@
+             [(WebAssemblycallseq_end timm:$amt1, timm:$amt2)],
+	     "#ADJCALLSTACKUP $amt1 $amt2">;
+
----------------
If we're going to use Pseudo instructions, should implement expandPostRAPseudo to expand them.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrCall.td:24
@@ +23,3 @@
+
+let Uses = [SP32, SP64], hasSideEffects = 1, isCall = 1 in {
+// FIXME: Can this use a multiclass like RETURN does? And simplify void?
----------------
It looks like it's not necessary to set hasSideEffects on call instructions.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrInfo.td:50
@@ +49,3 @@
+                                  SDT_WebAssemblyCallVoid,
+                                  [SDNPHasChain, SDNPSideEffect, SDNPVariadic]>;
+def WebAssemblycall      : SDNode<"WebAssemblyISD::CALL",
----------------
It looks like it's not necessary to set SDNPSideEffect here either.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrInfo.td:53
@@ -35,1 +52,3 @@
+                                  SDT_WebAssemblyCall,
+                                  [SDNPHasChain, SDNPSideEffect, SDNPVariadic]>;
 def WebAssemblyargument : SDNode<"WebAssemblyISD::ARGUMENT",
----------------
Ditto about SDNPSideEffect.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrInfo.td:58
@@ -38,2 +57,3 @@
                                  SDT_WebAssemblyReturn,
                                  [SDNPHasChain, SDNPSideEffect]>;
+def WebAssemblywrapper  : SDNode<"WebAssemblyISD::Wrapper",
----------------
For that matter, we shouldn't need it on WebAssemblyreturn either.


http://reviews.llvm.org/D12219