Page MenuHomePhabricator

[clang][WebAssembly] Implement support for table types and builtins
Needs ReviewPublic

Authored by pmatos on Nov 30 2022, 5:55 AM.

Details

Summary

This commit implements support for WebAssembly table types and
respective builtins. Table tables are WebAssembly objects to store
reference types. They have a large amount of semantic restrictions
including, but not limited to, only being allowed to be declared
at the top-level as static arrays of zero-length. Not being arguments
or result of functions, not being stored ot memory, etc.

This commit introduces the attribute((wasm_table)) to attach to
arrays of WebAssembly reference types. And the following builtins to
manage tables:

  • ref __builtin_wasm_table_get(table, idx)
  • void __builtin_wasm_table_set(table, idx, ref)
  • uint __builtin_wasm_table_size(table)
  • uint __builtin_wasm_table_grow(table, ref, uint)
  • void __builtin_wasm_table_fill(table, idx, ref, uint)
  • void __builtin_wasm_table_copy(table, table, uint, uint, uint)

This commit also enables reference-types feature at bleeding-edge.

This is joint work with Alex Bradbury (@asb).

Diff Detail

Event Timeline

pmatos created this revision.Nov 30 2022, 5:55 AM
Herald added a project: Restricted Project. · View Herald Transcript
pmatos requested review of this revision.Nov 30 2022, 5:55 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 30 2022, 5:55 AM
pmatos planned changes to this revision.Nov 30 2022, 5:57 AM

Please wait to review until the RFC (coming soon) is published.

pmatos updated this revision to Diff 479265.Dec 1 2022, 6:03 AM

Remove unnecessary attribute that lingered around after some refactoring.

pmatos added a comment.Dec 1 2022, 6:04 AM

This is ready for review.
RFC has been published here: https://discourse.llvm.org/t/rfc-webassembly-reference-types-in-clang/66939

Thanks to @tlively and @asb for the reviews.

pmatos updated this revision to Diff 487702.Mon, Jan 9, 11:39 PM

Rebase on top of main.

pmatos updated this revision to Diff 487747.Tue, Jan 10, 4:23 AM

Fix test broken after recent change to opt call. Rebase again on main.

aaron.ballman added inline comments.Wed, Jan 11, 8:09 AM
clang/include/clang/AST/ASTContext.h
1511 ↗(On Diff #487747)

Ah, I suggested WebAssembly in another review, but Wasm is also fine by me -- just pick a consistent term to use.

clang/include/clang/Basic/DiagnosticSemaKinds.td
2174–2178

We should combine these diagnostics with a %select rather than keep duplicating the same message.

This is unused and untested.

3036–3041

We should probably combine these as well.

This is unused and untested.

6932

This is untested.

11640

I know you copied the text from the matrix diagnostic, but that diagnostic does not follow our usual style rules.

Why an -m option instead of a -f option?

11641–11642

This is unused and untested.

11790–11793

You can combine these and reword to cannot form a %select{pointer|reference}0 to a WebAssembly table

11792

This is untested. I'll stop commenting on those at this point, please ensure you're testing all diagnostics you've added.

11795

Do you plan to support non-zero-length tables in the near future? I'm wondering if this should be reworded to remove the "currently supported" phrasing.

11798–11799
11803
11805

You should also add test coverage for the weird GNU binary conditional operator: https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html (I reworded the diagnostic to make that less awkward).

11808–11813

Combine

11825

I'm a bit shocked by the number of new diagnostics for this type as it seems incredibly restrictive and like the rules are going to be hard to understand. For example, you cannot use this type in an exception specification despite that being a compile-time property. Can you use it within a conditional explicit clause (https://godbolt.org/z/sn3G8xE3T)? It must be static, but can it be thread local?

Basically, it seems like this type is unlike basically any other type and we're going to have to carry a significant amount of extra code around to handle all the edge cases and those edge cases look a bit like whack-a-mole in practice.

clang/lib/AST/Type.cpp
2356–2361
llvm/include/llvm/CodeGen/WasmAddressSpaces.h
44

Add newline

pmatos updated this revision to Diff 489468.Mon, Jan 16, 2:11 AM

Updating tests after some changes to previous diagnostics.

pmatos marked 5 inline comments as done.Wed, Jan 18, 8:35 AM
pmatos added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
2174–2178

Right - this is no longer needed.

3036–3041

Sigh - again. Apologies. This was an initial patch that used matrix subscript to create a table subscript patch but was dropped and therefore is unused.

pmatos marked 2 inline comments as done.Wed, Jan 18, 8:36 AM
pmatos updated this revision to Diff 490208.Wed, Jan 18, 9:52 AM

Remove some unnecessary diagnostics.

pmatos marked 2 inline comments as done.Mon, Jan 23, 5:47 AM
pmatos added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
11795

Yes, the next phase of this work is to implement this together with element section to initialize tables.

asb added inline comments.Mon, Jan 23, 5:52 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11825

The whack-a-mole aspect of disallowing table uses is something I'm not fond of either....but I'm not sure I see a better approach. Do you have any alternatives in mind?

pmatos updated this revision to Diff 491353.Mon, Jan 23, 6:47 AM
pmatos marked 7 inline comments as done.

Address more comments.

pmatos updated this revision to Diff 491468.Mon, Jan 23, 11:34 AM

Fix some tests whose diagnostics changed

pmatos updated this revision to Diff 491608.Mon, Jan 23, 11:11 PM

Finish fixing tests after dianostic message changes

pmatos updated this revision to Diff 491707.Tue, Jan 24, 4:01 AM

Further tests for use of tables in conditional branches.

pmatos updated this revision to Diff 491784.Tue, Jan 24, 7:19 AM

Simplify and add tests for remaining diagnostics

pmatos marked an inline comment as done.Tue, Jan 24, 7:19 AM
pmatos marked an inline comment as done.Tue, Jan 24, 7:32 AM
pmatos added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
11825

A couple concrete answers in addition to what @asb already said.

  • I don't think we can use tables at the moment in conditional explicit clause. Indeed it doesn't make sense since there's no what, at the moment, to statically initialize the table.
  • Tables are thread local. Indeed, threads in WebAssembly share only linear memory so anything that's not in linear memory is thread local. This is true for tables but also for all reference types.
pmatos updated this revision to Diff 491790.Tue, Jan 24, 7:33 AM

Add missing newline at end of file.

@aaron.ballman Thanks for all the comments, I have now finished addressing those. What do you think of the current patch?