[PATCH] D12461: WebAssembly: generate load/store

[Dan Gohman via llvm-commits]

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

================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:103
@@ -91,1 +102,3 @@
+SmallString<32>
+WebAssemblyAsmPrinter::OpcodeName(const WebAssemblyInstrInfo *TII,
                                   const MachineInstr *MI) {
----------------
Is there a reason why OpcodeName has to be a member function?

================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:113
@@ -99,1 +112,3 @@
+  return ("$" + S).str();
+}
 
----------------
Ditto?

================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:136
@@ -116,3 +135,3 @@
 
-static std::string toString(const APFloat &FP) {
+std::string WebAssemblyAsmPrinter::toString(const APFloat &FP) {
   static const size_t BufBytes = 128;
----------------
And ditto here? Having this be a standalone function nicely emphasizes that it's just a pure function from APFloat to string.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrMemory.td:33
@@ +32,3 @@
+// Basic load.
+def LOAD_I32_ : I<(outs Int32:$dst), (ins Int32:$addr),
+                  [(set Int32:$dst, (load Int32:$addr))]>;
----------------
Nit: Why the trailing underscore in the name?

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrMemory.td:62
@@ +61,3 @@
+def LOAD_ZX_I32_I64_ : I<(outs Int64:$dst), (ins Int32:$addr),
+                         [(set Int64:$dst, (zextloadi32 Int32:$addr))]>;
+
----------------
I'm ok with expanding things out like this, but several other things in the WebAssembly tablegen files are aggressively factored out into multiclasses. Is there a reason these aren't multiclasses too?

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrMemory.td:71
@@ +70,3 @@
+
+// Basic store.
+def STORE_I32_  : I<(outs), (ins Int32:$addr, Int32:$val),
----------------
Please add a comment mentioning that WebAssembly's operand order is the opposite of SelectionDAG's operand order here.


http://reviews.llvm.org/D12461