Page MenuHomePhabricator

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

Authored by vchuravy on Aug 9 2019, 3:26 PM.

Details

Reviewers
loladiro
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
235

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
293

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
1204

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

llvm/lib/Object/WasmObjectFile.cpp
823

event -> table

995

Why should we only allow anyref?

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

Global -> Table

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

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
63

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

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

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

llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td
6
  • 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?
11

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.

13

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

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

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

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

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
56

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.

vchuravy commandeered this revision.Oct 2 2019, 1:31 PM
vchuravy added a reviewer: loladiro.
vchuravy updated this revision to Diff 222900.Oct 2 2019, 1:31 PM
  • change anyref AS to 256
  • fix some comments and address nit

Keno has asked me to take this over for him and I will work on getting this into shape so that it can get landed.

Another approach to reference types we should look at is clang's upcoming sizeless types support (https://reviews.llvm.org/D62962, RFC: http://lists.llvm.org/pipermail/cfe-dev/2019-June/062523.html). This would allow reference types to be constructed at the source level but most operations such as sizeof, loading, and storing would be disallowed because they would be treated as incomplete types.

I don't think that approach is inconsistent with this one, though. This patch deals mostly with the backend while sizeless types are a frontend concept. I'm not sure what the codegen and lowering would look like, though.

It would be great to see some test .ll files here so we can get a better understanding for what is implemented here. I would also like to see tests of what happens if you try to do a ptrtoint or some other illegal operation on a reference type.

llvm/lib/Target/WebAssembly/WebAssembly.h
88

I assume other reference types will get their own address spaces so we can differentiate them in IR. Would it then make sense to put the table itself in another address space? We should also think about how best to partition the address space space looking forward to when we will have multiple memories, multiple tables, and multiple function references or reference type imports.

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

I think this message needs to be updated for the new AS numbers.

One other thought: Does the implementation in this diff care what type the reference type pointers are pointing to? It would be nice if we could enforce the use of an opaque pointee type to prevent the reference types from being dereferenced.

vchuravy updated this revision to Diff 231343.Nov 27 2019, 7:42 PM
vchuravy marked 6 inline comments as done.
  • add test for passing anyref through a function
  • fix wrong name for funcref
  • fixes and formatting
  • fix error message
vchuravy marked 4 inline comments as done.Nov 27 2019, 7:52 PM

Rebased onto current master and added an initial test. (I will start adding more as I start integrating this with the rest of the toolchain)

llvm/lib/Object/WasmObjectFile.cpp
995

Not sure what Keno meant by this comment, but it might be to check that reference-types is enabled before allowing it as a element type.

llvm/lib/Target/WebAssembly/WebAssembly.h
88

Yes. funcref should get its own address space but I am not sure about its usage yet.

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

The question is which of these types need representing. As far as I can tell exnref and is produced by LLVM already and funcref is primarily used for function tables in modules and isn't necessarily a frontend facing time. Albeit nothing in the spec says that it couldn't be.

If funcref would need a frontend facing representation it would need a new AS.

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

I don't see why it shouldn't without activating -mattr=+reference-types new .ll files won't compile,
but old one should.

vchuravy updated this revision to Diff 231460.Nov 28 2019, 12:08 PM

support old DL modules

vchuravy updated this revision to Diff 231462.Nov 28 2019, 12:18 PM

restore previous changes

vchuravy marked an inline comment as done.Nov 28 2019, 12:23 PM
vchuravy added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
144

I see what you meant know after building emscripten/binaryen with these changes, I made the DL changes depending on the feature flag.

vchuravy updated this revision to Diff 231601.Nov 30 2019, 2:59 PM
  • fix AS in anyref testfile