Page MenuHomePhabricator

[WebAssembly] WIP: Add support for reference types
Needs ReviewPublic

Authored by loladiro on Aug 9 2019, 3:26 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

This is a quick-and-dirty straw man implementation of reference-types
support for the WebAssembly backend. anyref value types are represented
as non-integral pointers in addrspace(1) and tables are represented as
external global in addrspace(1) pointing to pointers also in addrspace(1).

The bulk of the work here is just adding all the boiler plate for multiple tables
and the new instruction encodings. The ISel part of this is very primitive
and just the quickest thing I could possibly do to play with this. In its current
state, I've gotten it to emit .o files that I think are correctly formed,
but I haven't been able to test them properly (I think at least some of the
problem is browser bugs I'm looking into).

This isn't ready for review or anything, but I figured
a) This might save somebody else some time in implementing the boiler plate
b) It could serve as a starting point to discuss how to represent this in IR

Event Timeline

loladiro created this revision.Aug 9 2019, 3:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 3:26 PM
loladiro updated this revision to Diff 214467.Aug 9 2019, 3:29 PM

Accidentally only pushed half the changes

Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 3:29 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Harbormaster completed remote builds in B36554: Diff 214467.
fitzgen added a subscriber: fitzgen.Aug 9 2019, 3:38 PM
tlively added a subscriber: tlively.Aug 9 2019, 4:22 PM

Thank you! We also have been floating some ideas about supporting reference types as types in another address space, but haven't actually started implementing it AFAIK. I like the general direction. Let's wait for other people's comments too.

  • Are you planning to land this at some point (after adding some tests and such), or is this purely for discussions for the direction? If it's the latter, maybe I don't need to comment in the code in too much detail.. But anyway I did, so please ignore them if you are not actually planning to land this.
  • Even if not to the extent to cover the whole CL, I think a few test cases still would be helpful for discussions to see how reference types can be represented in .ll files and printed in .s files.
  • There are many places that add anyref but not funcref.
  • Out of curiousity, is Julia planning to use reference types? If so, for which concept?
  • I know it is not finished, but please clang-format if you are gonna land this later.
  • Maybe you've already seen this, but if we end up landing this, we should update the linking spec in the tool convention, because we add a new relocation type.
lld/wasm/WriterUtils.cpp
186

Why not funcref?

llvm/include/llvm/BinaryFormat/WasmRelocs.def
18

I guess this is for the index to distinguish tables, not elements within the table, right? The name is not very distinguishable with R_WASM_TABLE_INDEX_SLEB and R_WASM_TABLE_INDEX_I32 above, so I guess we need a new name or something

llvm/include/llvm/MC/MCExpr.h
296

Do we need this? Other symbol kinds (global, function, and events) don't have corresponding VK_WSM_ entries here. VK_WASM_TYPEINDEX is left for some other reason (D58472).

llvm/lib/MC/WasmObjectWriter.cpp
1233

Can't this be other reference types, like FUNCREF?

llvm/lib/Object/WasmObjectFile.cpp
823

event -> table

998

Why should we only allow anyref?

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

Global -> Table

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1125

It looks it assumes at this point we are not PIC. Maybe we can assert in if (isPositionIndependent()) { above that nonzero address space is not supported?

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h
54

How are we gonna represent other reference types, such as funcref or exnref?

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

Not sure what this means. Isn't this the type of table itself, not that of the elements?

llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td
5
  • Nit: For these two lines: We usually don't seem to have a whitespace after : for *.td files
  • Is the reason for using offset instead of index here that you plan to use named operand table methods?
10

I guess the name should be anyref.table.get to follow the wasm backend convention (even though we don't have separate instructions). And because "table.get" part is the constant, we only need to parameterize the prefix. This can be similar to something like here.

12

I guess rules to translate instructions for non-zero indices have not been implemented here, right?

llvm/lib/Target/WebAssembly/WebAssemblyRegisterInfo.td
47

Real nit: maybe we don't need a newline between reference types

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
113

Is this gonna support existing .ll files with the previous data layout?

wingo added a subscriber: wingo.Aug 12 2019, 6:38 AM

Overall direction looks good to me as well.

Ditto that tests would be useful just to make it clearer how this is represented in IR.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h
47

We'll almost-surely want to pull this 1 out into a constant, WasmAnyrefAddressSpace or something

x86 uses address spaces starting at 256 and counting up for its architecture-specific address spaces. The docs say "Address spaces 1-255 are currently reserved for user-defined code." so we should consider using 256 here.