Page MenuHomePhabricator

[WebAssembly] Implement table instruction intrinsics and ref.null
ClosedPublic

Authored by pmatos on Nov 8 2021, 10:30 AM.

Details

Summary

This change implements intrinsics for ref.null, table.grow,
table.fill, table.size and table.copy.

Diff Detail

Event Timeline

pmatos created this revision.Nov 8 2021, 10:30 AM
pmatos requested review of this revision.Nov 8 2021, 10:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2021, 10:30 AM
pmatos updated this revision to Diff 386842.Nov 12 2021, 7:00 AM

After D111227 was refactored to just contain the polymorphic type implementation, I refactored this revision to contain all the implementation of the table intrinsics and ref.null

tlively added inline comments.Nov 14 2021, 10:47 PM
llvm/lib/CodeGen/ValueTypes.cpp
203–206

I think we should probably simplify things by making externref be an i8 pointer as well. That's simpler than the other arbitrary type (pointer (to pointer?) to opaque struct) that we are using now, and ultimately the type doesn't really matter. Maybe these comments should say "arbitrary pointer" rather than "opaque pointer" (which I don't think is quite correct, since it's the struct that's opaque rather than the pointer) or "void pointer" (which is not really a concept that exists in LLVM).

llvm/lib/IR/Function.cpp
927 ↗(On Diff #386842)

The changes in this file look like they should be in the previous revision rather than this one. Is that right?

llvm/lib/Target/WebAssembly/WebAssemblyInstrRef.td
14 ↗(On Diff #386842)

Since the rc, vt, and ht all correspond 1:1 with each other, you can package them together into records that let you just have a single parameter here. For an example of this pattern, see class Vec and all its instantiations in WebAssemblyInstrSIMD.td.

llvm/test/CodeGen/WebAssembly/funcref-call.ll
2–3

I'm not sure it should be necessary to modify these tests to use i8 as the pointee type. Ultimately the pointee type shouldn't really matter at all (except at the point when a funcref is called) and we should be relying entirely on the address space to figure out what Wasm type it should be. In fact, it would be useful to test that multiple pointee types work correctly. That will give us confidence that nothing will break when opaque pointers are enabled.

llvm/test/CodeGen/WebAssembly/table-copy.ll
21

Can you skip the previous getelementptr and cast the original table pointer directly to i8 addrspace(1)*, at least when the offset is 0? It would also be good to test with various nontrivial offsets and when copying from a table to itself. Same for the other intrinsics.

llvm/utils/TableGen/IntrinsicEmitter.cpp
197 ↗(On Diff #386842)

These changes also looks like they should be in the previous revision rather than this one.

pmatos updated this revision to Diff 389494.Nov 24 2021, 7:36 AM
pmatos retitled this revision from [WebAssembly] Implementation of intrinsics for table.fill and table.copy to [WebAssembly] Implement table instruction intrinsics and ref.null.
pmatos edited the summary of this revision. (Show Details)

After closing D111420, I moved the implementation of all intrinsics to this revision without the need for the polymorphic llvm_anyref_ty type.

There's a single niggle here that I haven't been able to fix and would like further comments on.

Test for reference-types.s is failing due to the parsing of ref.null $heaptype, be it func or extern.
The reason for this is failing is that I have removed the need to specify the input argument heaptype for the ref.null instruction. This allows me to define an intrinsic that doesn't receive an argument. If I specify a heaptype as an input argument, then TableGen really wants the argument to show up in the instruction pattern, but I would really like it to be ignored. Adding extern or func as an argument to the intrinsic just seems overcomplicated. On the other hand, I have been stuck with this for a while and would appreciate some fresh ideas.

pmatos planned changes to this revision.Nov 25 2021, 6:58 AM
pmatos added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyInstrRef.td
18 ↗(On Diff #389494)

The problem I alluded to in https://reviews.llvm.org/D113420#3151549 happens here. Because I don't mention heaptype as inputs, the parser fails. But if I do add as inputs heaptype, and not in the instruction pattern (where the intrinsic is), then tablegen does not accept it. It makes, however, no sense to have heaptype as an argument since it can be suffix to the intrinsic name.

pmatos updated this revision to Diff 391058.Dec 1 2021, 9:18 AM

Post discussion with @tlively I have changed the textual representation of ref.null to include the heaptype as suffix.
As a consequence the current patch includes all the intrinsics implementations but also the changes to the tests that now use ref.null_extern and ref.null_func, and the removal of the references to HeapType which is now not required anymore.

pmatos planned changes to this revision.Dec 2 2021, 6:53 AM

To make the changes easier to understand, I am splitting this into two. One for ref.null + HeapType changes and another for the table intrinsics.

pmatos updated this revision to Diff 391468.Dec 2 2021, 2:41 PM

Remove changes related to ref.null.

ref.null intrinsic implementation and heaptype removal work is currently in D114979 and this change builds upon that.

pmatos added inline comments.Dec 6 2021, 3:08 AM
llvm/lib/CodeGen/ValueTypes.cpp
203–206

I agree that the comments should be changed, but don't you think that externref should still be a pointer to an opaque struct? This seems to match better with what externref truly represents: a host value that you cannot introspect - therefore opaque. I am not sure it matters much as you say, but semantically it seems to be closer to what externref is.

llvm/test/CodeGen/WebAssembly/table-copy.ll
21

Good point! I will take a look at these.

pmatos added inline comments.Dec 6 2021, 5:18 AM
llvm/test/CodeGen/WebAssembly/funcref-call.ll
2–3

The reason I changed this for all tests is for consistency with intrinsics tests. Intrinsics tests need to define the funcref as i8 pointers to addspace(20), because the intrinsics validator post parse checks that the type defined in LLVM IR matches the type of the intrinsic. Even though it might not be necessary to change it in this specific testcase, I find that for consistency we should always define the funcref type as a type i8 addspace(20).

pmatos updated this revision to Diff 392034.Dec 6 2021, 5:39 AM

Fix a couple of comments - add a new test to table.copy for copies within the same table.

llvm/test/CodeGen/WebAssembly/table-copy.ll
30

@tlively is this the kind of test you were referring to?

tlively accepted this revision.Dec 6 2021, 9:07 AM

LGTM with the extra suggested test, if possible.

llvm/test/CodeGen/WebAssembly/funcref-call.ll
2–3

I agree that it makes sense to be consistent in the majority of the tests, but perhaps we could have an additional test that only checks that the pointee type doesn't matter? That will help us be confident that we will be ready for the removal of pointee types once the wider project makes that switch.

llvm/test/CodeGen/WebAssembly/table-copy.ll
30

Looks good! I was probably thinking of having different offsets in the getelementptr, but not supporting that for now and taking the offsets as arguments to the copy intrinsic seems much simpler.

This revision is now accepted and ready to land.Dec 6 2021, 9:07 AM
pmatos added inline comments.Dec 7 2021, 2:08 AM
llvm/test/CodeGen/WebAssembly/table-copy.ll
30

Humm, I see what you mean but isn't it always the case that since we are dereferencing a global, the GEP will always have zero offsets? I cannot imagine a case, where we use non-zero offsets to dereference a global representing a table, like we do for example in the llvm/test/CodeGen/WebAssembly/funcref-tableget.ll test.

This revision was landed with ongoing or failed builds.Dec 7 2021, 4:26 AM
This revision was automatically updated to reflect the committed changes.
ljmf00 added a subscriber: ljmf00.Wed, Jan 12, 6:06 PM

After closing D111420, I moved the implementation of all intrinsics to this revision without the need for the polymorphic llvm_anyref_ty type.

@pmatos You likely referenced the wrong diff. I would suggest amending this to avoid confusion in the future :)