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

[Paulo Matos via Phabricator via llvm-commits]

pmatos 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)
----------------
tlively wrote:
> 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).
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.


================
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
----------------
tlively wrote:
> 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.
Good point! I will take a look at these.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113420