[PATCH] D101656: [WebAssembly] Fixup order of ins variables for table instructions

Sat May 1 07:05:06 PDT 2021

sbc100 added a comment.

In D101656#2731245 <https://reviews.llvm.org/D101656#2731245>, @pmatos wrote:

> In D101656#2730556 <https://reviews.llvm.org/D101656#2730556>, @dschuff wrote:
>
>> I think all of emscripten's use of the table is still via the JS API (e.g. src/runtime_functions.js) so this probably won't break emscripten. It does seem likely that nobody is using these instructions via assembly.
>> Is this the only change needed? Do we have no tests of these asm instructions anywhere in LLVM at all?
>
> We don't have tests for the register based version of the assembly string afaiu. 
> It would be great to be able to run unit tests that build machine instructions and check the expected asm, however I don't think there's an infrastructure for that. If there is, I would be happy to add those tests.
> Another reason we didn't notice this was broken is because we do not execute this tests, otherwise we would have seen the values and indexes for the table set and others were incorrectly ordered. I am wondering if a wasm validator would have caught this, however even if it does as far as I understand the binaryen validator does not parse LLVM assembly files.

Don't tests like the ones in https://github.com/llvm/llvm-project/blob/8a5e0d956396f07d2241091693f8a714a2483356/llvm/test/MC/WebAssembly/tables.s#L25 at least get both the asm output and the binary output?

Its true and we currently lack any kind of validation of execution engine in the llvm tests.. but that is also true for all other target IIUC.   I guess we are different from something like x86 because we at least have the potential to validate..   Perhaps we should have least have some kind of FYI bot that attempt to validate all the test output of wasm-ld... but I don't think that lack of this has been a huge problem in the past.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101656/new/

https://reviews.llvm.org/D101656