Page MenuHomePhabricator

[WebAssembly] Implementation of (most) table instructions
ClosedPublic

Authored by pmatos on Oct 20 2020, 8:25 AM.

Details

Summary

Implementation of instructions table.get, table.set, table.grow,
table.size, table.fill, table.copy.

Missing instructions are table.init and elem.drop as they deal with
element sections which are not yet implemented.

Added more tests to tables.s

Diff Detail

Event Timeline

pmatos created this revision.Oct 20 2020, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2020, 8:25 AM
pmatos requested review of this revision.Oct 20 2020, 8:25 AM

So the goal here is to add most of the table instructions. This introduces some table relocations, which (in a following patch) we will resolve in the linker.

I have some concerns with this patch. The tests here are testing that instructions table.* written in .s files will be assembled properly into an object file. However, there are no tests actually testing the register based part of the instruction definitions.

So, for example for table.grow, I am unsure that

(outs i32imm_op:$sz), (ins table32_op:$table, i32imm_op:$n, reftype_op:$val)

actually makes sense for the register based version of the instruction and I have no tests actually looking into it since I have been mostly concerned at assembler level.
I assume that we cannot test this atm since register-level is relevant at the codegen part and we have no way to represent in IR a table.grow operation.

Should we add intrinsics for these? and try to test this at codegen level? or just add a comment pointing out that register-level instruction definitions are currently untested? or something else? Suggestions welcome.

Should we add intrinsics for these? and try to test this at codegen level? or just add a comment pointing out that register-level instruction definitions are currently untested? or something else? Suggestions welcome.

Yes, I would typically add intrinsics for something like this and test codegen from them. This would require being able to represent reference types in LLVM IR, though, so I would be ok with leaving that to the future. For now if you pass -show-encoding to llvm-mc you can at least test that the binary encoding is what you expect it to be. See simd-encodings.s for an example of that.

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td
169–170

Rather than a new Operand, this should probably be a new register class defined in WebAssemblyRegisterInfo.td. It will also probably make sense for now to have two new register types, one for externref and one for funcref rather than trying to combine them. Then there should be two versions of each table instruction, one for externref and one for tableref, even though those will lower to the same WebAssembly instruction. For an example of something similar, see LOCAL_GET_* in WebAssemblyInstrInfo.td.

llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td
25

$i should be an I32 (i.e. an i32 stack value), not an i32imm_op (i.e. a constant immediate argument). Same for i32s in the instructions below. Similarly, $val should be either EXTERNREF or FUNCREF once those register classes are defined.

In general I'm loving how small this change is. I don't really grok the tablegen stuff but everything else looks pretty good to me.

llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
201

This doesn't need the 32 suffix.. i think its more like OPERAND_GLOBAL. I can't imagine us ever needing 2^^64 different tables... touch wood.

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
79

I think "table index" in ambiguous. Maybe "table number"?

llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td
15

These two comments above can probably just be a single comment, no? (unless I'm missing something?)

66

Remove trailing newlines.

llvm/test/MC/WebAssembly/tables.s
39

I think its common to put the expectations for each function inside or alongside the function itself.

But I admit we are not very consistent in this directory.

What do you think about the style uses in MC/WebAssembly/bulk-memory-encodings.sor simd-encodings.s.

@tlively probably knows more about the right and wrong way to do this stuff.

tlively added inline comments.Oct 20 2020, 10:57 PM
llvm/test/MC/WebAssembly/tables.s
39

Yeah, I would probably put the expected assembly output for each function just above that function and leave all the YAML at the bottom, but I agree that we aren't too consistent with this.

pmatos marked 5 inline comments as done.Oct 21 2020, 6:10 AM

Thanks for the review - I will submit a new revision.

llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
201

Ah - didn't Gates once said something similar regarding amount of required RAM? ;-)
Famous last words...

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
79

Agree.

pmatos marked an inline comment as done.Oct 21 2020, 6:10 AM
pmatos updated this revision to Diff 299666.Oct 21 2020, 6:28 AM

These changes reflect the first set of comments. High on the list is quite a few changes related to setting registers for funcrefs and externrefs. A few things might not be strictly necessary but pave the way for future work.

I was tempted to implement the intrinsics, but I think I will leave for a next patch. Sam mentions one of the things he likes about the patch is that it's small, so lets not mess with that. :)

Regarding linter clang-format suggestions - should I reformat the code as suggested or leave it?
Some suggested changes are not necessarily on code I wrote, which is why I ask.

No I wouldn't format any code you didn't write.. nor assume that clang-format always knows best in all cases.

I normally run git clang-format origin/master to ensure that the lines I've touched are formatted correctly.

sbc100 added inline comments.Oct 21 2020, 8:24 AM
llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td
316

How is that we were able to get away without these types of changes when implementing the global.get and global.set of externref symbols in llvm/test/MC/WebAssembly/externref.s?

There is have instructions that take externref such as global.set my_global.

pmatos added inline comments.Oct 21 2020, 9:36 AM
llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td
316

Yes - at first glance, that shouldn't work. But it does work without the changes in this patch so I am stumped and might have to do some looking up to understand it. Maybe @tlively knows more?

tlively added inline comments.Oct 21 2020, 12:45 PM
llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td
316

Ah, this is interesting! Remember that llvm-mc only ever uses the stack version of instructions at the MC layer, so in that test the global.get and global.set instructions never actually have any operands of any register class. We also never use global.get or global.set of reference types in any of our instruction selection, so this never came up.

At a deeper level, this worked because register classes in the WebAssembly backend are fundamentally nothing more than a debugging tool. Since we don't do register allocations and because all register values are erased before we emit modules, everything would continue working correctly if just had a single register class covering all value types. The benefit of having separate register classes is that it enables the MachineInstrVerifier to catch type errors for us. If we actually had a wasm validator in the backend, the separate register classes would not be useful for anything except documentation.

Is this good to merge?

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td
316

That makes sense. Thanks for the thorough explanation.

pmatos updated this revision to Diff 299909.Oct 22 2020, 3:30 AM

Remove unused operand type.

This mostly lgtm. I would love it if we could avoid adding the new MVT value for now since then we could avoid changing the shared code on llvm/lib/CodeGen/ValueTypes.cpp.. but perhaps that absolutely needed for this change?

lgtm if @tlively is OK with the .td changes.

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td
316

So can we get away without adding these for now?

llvm/utils/TableGen/CodeGenTarget.cpp
218

Maybe use the local style of putting these on single line?

tlively accepted this revision.Oct 22 2020, 9:36 PM

Looks good to me with @sbc100's final comment addressed 🎉

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td
316

I think it would be better to land the new MVTs and register classes before or at the same time as instructions that are going to use them. Even if they aren't fundamentally important, we've been treating them as no less important than they would be in other backends and being properly consistent with their usage will avoid any future surprises.

This revision is now accepted and ready to land.Oct 22 2020, 9:36 PM
pmatos added inline comments.Oct 22 2020, 11:09 PM
llvm/utils/TableGen/CodeGenTarget.cpp
218

I will do that but this was a lint suggested change, so I accepted it. I will revert.

pmatos updated this revision to Diff 300190.Oct 23 2020, 1:30 AM

Align with local indent style

@sbc100 if it's ok for you, feel free to merge, thanks.

This revision was landed with ongoing or failed builds.Oct 23 2020, 8:43 AM
This revision was automatically updated to reflect the committed changes.