[PATCH] D48183: [WebAssembly] Modified tablegen defs to have 2 parallel instuction sets.

Wouter van Oortmerssen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 14 15:40:14 PDT 2018


aardappel added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrFormats.td:15
 
 // WebAssembly Instruction Format.
+class WebAssemblyInst<bits<32> inst, string asmstr, bit stack> : Instruction {
----------------
dschuff wrote:
> There should probably be a bit more explanation here, saying that every instruction has the 2 representations, how they are used, how the oops/iops and asmstrings need to be different, the naming convention, etc.
Ok, added more description.


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrMemory.td:157
+defm LOAD16_S_I64 : WebAssemblyLoad<I64, "i64.load16_s", 0x32>;
+defm LOAD16_U_I64 : WebAssemblyLoad<I64, "i64.load16_u", 0x32>;
+defm LOAD32_S_I64 : WebAssemblyLoad<I64, "i64.load32_s", 0x34>;
----------------
jgravelle-google wrote:
> Why did the the binary encoding change from 0x33 to 0x32 here?
I have NO idea how that happened. But thanks for spotting a potentially costly bug. Also we need better tests, apparently.


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrMemory.td:467
+                    Requires<[HasAddr32]>;
+// FIXME: add $flags to first asmstr.
+defm CURRENT_MEMORY_I32 : I<(outs I32:$dst), (ins i32imm:$flags),
----------------
dschuff wrote:
> The real FIXME is to just remove CURRENT_MEMORY and GROW_MEMORY altogether (they are the old names) assuming we can remove any uses from e.g. Rust and clang.
Ok, removed the FIXMEs for now.


Repository:
  rL LLVM

https://reviews.llvm.org/D48183





More information about the llvm-commits mailing list