[PATCH] D113420: [WebAssembly] Implementation of intrinsics for table.fill and table.copy

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/CodeGen/ValueTypes.cpp:203-205
   case MVT::externref:
-    return PointerType::get(StructType::create(Context), 10);
+    return PointerType::get(StructType::create(Context),
+                            10); // opaque pointer to addrspace(10)
----------------
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).


================
Comment at: llvm/lib/IR/Function.cpp:927
 /// NOTE: This must be kept in synch with the copy in TblGen/IntrinsicEmitter!
 enum IIT_Info {
   // Common values should be encoded with 0-15.
----------------
The changes in this file look like they should be in the previous revision rather than this one. Is that right?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrRef.td:14
 
-multiclass REF_I<WebAssemblyRegClass rc, ValueType vt> {
-  defm REF_NULL_#rc : I<(outs rc:$res), (ins HeapType:$heaptype),
-                        (outs), (ins HeapType:$heaptype),
-                        [],
-                        "ref.null\t$res, $heaptype",
-                        "ref.null\t$heaptype",
+multiclass REF_I<WebAssemblyRegClass rc, ValueType vt, string ht> {
+  defm REF_NULL_#rc : I<(outs rc:$dst), (ins),
----------------
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.


================
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
----------------
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.


================
Comment at: llvm/test/CodeGen/WebAssembly/table-copy.ll:20
+  %tableptr1 = getelementptr [0 x %externref], [0 x %externref] addrspace(1)* @externref_table1, i32 0, i32 0
+  %tb1 = bitcast %externref addrspace(1)* %tableptr1 to i8 addrspace(1)*
+  %tableptr2 = getelementptr [0 x %externref], [0 x %externref] addrspace(1)* @externref_table2, i32 0, i32 0
----------------
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.


================
Comment at: llvm/utils/TableGen/IntrinsicEmitter.cpp:197
 // NOTE: This must be kept in synch with the copy in lib/IR/Function.cpp!
 enum IIT_Info {
   // Common values should be encoded with 0-15.
----------------
These changes also looks like they should be in the previous revision rather than this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113420