[PATCH] D45848: [WebAssembly] Initial Disassembler.

Thu May 10 14:39:45 PDT 2018

aardappel added inline comments.

================
Comment at: lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:170
+      case MCOI::OPERAND_REGISTER: {
+        // These are NOT actually in the instruction stream, but MC is going to
+        // expect operands to be present for them!
----------------
dschuff wrote:
> aardappel wrote:
> > dschuff wrote:
> > > I think we will need to generate the register assignments, either here or in a post-processing step. I don't think it currently makes sense to have a second version of all the instructions with implicit operands (even though they wouldn't be needed by MC in explicit-locals mode).
> > > 
> > > You'd think it ought to be as simple as running a "pass" over the MCInsts, but  last I checked there wasn't really any facility for running passes over MC; it's structured much more like a stream. (and looking at the use of `getInstruction` in  e.g. LLVMDisasmInstruction in MCDisassembler or in llvm-objdump, it looks like the returned MCInsts just get passed directly through to the printer. We could maybe hang some state off of the WebAssemblyDisassembler?
> > Yes, we can easily keep a stack to track assignment + types in WebAssemblyDisassembler, but we'd be making the assumption that instructions are always disassembled in order. To accomodate this code being called for disassembling single instructions, this code would have to be "robust" and still function if the stack is empty etc.
> > 
> > More-over to get correct types we'd need access to locals/args, which we don't have. We currently don't even know where a function starts and stops.
> > 
> > Then, in table-gen there is currently one instruction per return type, i.e. i32.call vs i64.call, which are actually the same instruction. We'd either need to strip these types in a post-process, or we'd need to output more complicated tables that contain all of them, so the dissassembler can pick the right one based on stack type information, if it has any.
> > 
> > Since this isn't quite trivial, I suggest I land this basic disassembler (which will at least allow objdump etc to show something useful), then experiment with the above in follow-ups.
> > More-over to get correct types we'd need access to locals/args, which we don't have. We currently don't even know where a function starts and stops.
> 
> Ah yeah, that's the really unfortunate bit that I hadn't thought about.
> 
> > Then, in table-gen there is currently one instruction per return type, i.e. i32.call vs i64.call, which are actually the same instruction. 
> This is mostly just for call, right? It looks all the flavors have the same mnemonic, so maybe we can just fake it or something.
> 
> > Since this isn't quite trivial, I suggest I land this basic disassembler (which will at least allow objdump etc to show something useful), then experiment with the above in follow-ups.
> 
> Yeah I think that makes sense. This seems like a reasonable unit of code to put in, do you want to remove the DO NOT MERGE and consider it ready then? I like that this is one of the few areas of LLVM that can get proper unit tests. We should maybe also add at least a basic mc test as well though to ensure things can be hooked up end-to-end. See e.g. test/MC/Disassembler, but we can probably put the most coverage in the unit tests.
You're right, it is actually few instructions: call, call_indirect, select. The first two already have a shorter version in the tables, so if I on purpose select the one with the least operands, only select remains? So maybe we can add an instruction for that one.. then we can largely ignore types.

I have a bit more testing I want to do with objdump before we can merge this, I will let y'all know when its ready.

There are already 2 tests here, one in unittests/MC/Disassembler.cpp and one in test/MC/Disassembler/WebAssembly/wasm.txt, see below. The latter in particular could use more tests I am sure.

Repository:
  rL LLVM

https://reviews.llvm.org/D45848