[PATCH] D113420: [WebAssembly] Implement table instruction intrinsics and ref.null

[Thomas Lively via Phabricator via llvm-commits]

tlively accepted this revision.
tlively added a comment.
This revision is now accepted and ready to land.

LGTM with the extra suggested test, if possible.



================
Comment at: llvm/test/CodeGen/WebAssembly/funcref-call.ll:7-8
 define void @call_funcref(%funcref %ref) {
-  call addrspace(20) void %ref() 
+  %f = bitcast %funcref %ref to %funcptr
+  call addrspace(20) void %f() 
   ret void
----------------
pmatos wrote:
> tlively wrote:
> > 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.
> 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)`.
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.


================
Comment at: llvm/test/CodeGen/WebAssembly/table-copy.ll:29
+; Copies len items from table1 at src to table1 at src+off
+define void @self_table_copy(i32 %src, i32 %off, i32 %len) {
+; CHECK-LABEL: self_table_copy:
----------------
pmatos wrote:
> @tlively is this the kind of test you were referring to?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113420/new/

https://reviews.llvm.org/D113420