[WebAssembly] Fixup order of ins variables for table instructions

[pmatos]

Summary

WebAssembly instruction arguments should have their arguments ordered from
the deepest to the shallowest on the stack.

[tlively]

@sbc100 Will fixing the assembly format to have the correct operand order like this break Emscripten at all? I think the incorrect order also made it into the LLVM 12 release, but I think we have few enough users that it's probably worth fixing now.

[dschuff]

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?

[sbc100]

I don't think there are any uses of these instructions in emscripten and I doubt there are any emscripten users using the assembly format.

@sunfishcode do you know of anyone using these assembly instructions?

[pmatos]

In 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.

[pmatos]

In D101656#2731010, @sbc100 wrote:

I don't think there are any uses of these instructions in emscripten and I doubt there are any emscripten users using the assembly format.

@sunfishcode do you know of anyone using these assembly instructions?

Feel free to merge if you think this is ready to land - I should probably ask for committers permissions at some point.

[sbc100]

In D101656#2731245, @pmatos wrote:

In 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.

[tlively]

Oh right, this only changes the register-based printing, which is used exclusively in llc tests. The stacky assembly that would be used in production is unaffected, so it's not surprising that none of the existing assembly tests are affected.

I'm fine merging this as-is. Presumably once we have a way to generate these instructions from LLVM IR, we will also have tests for them reflecting this change.

[pmatos]

Merged by @tlively as https://github.com/llvm/llvm-project/commit/cd460c4d11eeeca485353ad6a1961b543980b664
In the meantime I got committers rights. Thanks.

llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td

 multiclass TABLE<WebAssemblyRegClass rt> {
   defm TABLE_GET_#rt : I<(outs rt:$res), (ins table32_op:$table),
                          (outs), (ins table32_op:$table),
                          [],
                          "table.get\t$res, $table",
                          "table.get\t$table",
                          0x25>;
 
-  defm TABLE_SET_#rt : I<(outs), (ins table32_op:$table, rt:$val, I32:$i),
+  defm TABLE_SET_#rt : I<(outs), (ins table32_op:$table, I32:$i, rt:$val),
                          (outs), (ins table32_op:$table),
                          [],
-                         "table.set\t$table, $val, $i",
+                         "table.set\t$table, $i, $val",
                          "table.set\t$table",
                          0x26>;
 
-  defm TABLE_GROW_#rt : I<(outs I32:$sz), (ins table32_op:$table, I32:$n, rt:$val),
+  defm TABLE_GROW_#rt : I<(outs I32:$sz), (ins table32_op:$table, rt:$val, I32:$n),
                           (outs), (ins table32_op:$table),
                           [],
-                          "table.grow\t$sz, $table, $n, $val",
+                          "table.grow\t$sz, $table, $val, $n",
                           "table.grow\t$table",
                           0xfc0f>;
 
-  defm TABLE_FILL_#rt : I<(outs), (ins table32_op:$table, I32:$n, rt:$val, I32:$i),
+  defm TABLE_FILL_#rt : I<(outs), (ins table32_op:$table, I32:$i, rt:$val, I32:$n),
                           (outs), (ins table32_op:$table),
                           [],
-                          "table.fill\t$table, $n, $val, $i",
+                          "table.fill\t$table, $i, $val, $n",
                           "table.fill\t$table",
                           0xfc11>;
 
 }
 
 defm "" : TABLE<FUNCREF>, Requires<[HasReferenceTypes]>;
 defm "" : TABLE<EXTERNREF>, Requires<[HasReferenceTypes]>;
 
 defm TABLE_SIZE : I<(outs I32:$sz), (ins table32_op:$table),
                     (outs), (ins table32_op:$table),
                     [],
                     "table.size\t$sz, $table",
                     "table.size\t$table",
                     0xfc10>,
                     Requires<[HasReferenceTypes]>;
 
 
-defm TABLE_COPY : I<(outs), (ins table32_op:$table1, table32_op:$table2, I32:$n, I32:$s, I32:$d),
+defm TABLE_COPY : I<(outs), (ins table32_op:$table1, table32_op:$table2, I32:$d, I32:$s, I32:$n),
                     (outs), (ins table32_op:$table1, table32_op:$table2),
                     [],
-                    "table.copy\t$table1, $table2, $n, $s, $d",
+                    "table.copy\t$table1, $table2, $d, $s, $n",
                     "table.copy\t$table1, $table2",
                     0xfc0e>,
                     Requires<[HasReferenceTypes]>;