[PATCH] D101140: [WebAssembly][CodeGen] Allow for externref/funcref local variables

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

Nice! This turns out to be relatively simple. It would be good to add more tests with reference typed locals mixed in with normal locals, etc. Also, what happens when one of the alloca values is used by something other than a load or a store? Is it just an ISel failure at that point?



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td:101
                                  SDT_WebAssemblyArgument>;
+def WebAssemblylocalGet : SDNode<"WebAssemblyISD::LOCAL_GET",
+                                 SDT_WebAssemblyLocalGet,
----------------
I would name this `WebAssemblylocal_get` to match `WebAssemblybr_table`, although admittedly the naming convention for these custom nodes is very weird.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrRef.td:29
                     Requires<[HasReferenceTypes]>;
+
 }
----------------
Unintentional whitespace change?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.h:107
+      // responsible for marking the stack object as dead in the frame.
+      FrameIndex2Local.insert(std::make_pair(FrameIndex, LocalIdx));
+      addLocal(VT);
----------------
I believe this should work fine as well. In fact this, whole pattern could be reduced to this:

```
auto Results = FrameIndex2Local.insert({FrameIndex, Locals.size()});
if (Results.second)
  addLocal(VT);
return Results.first->second + Params.size();
```



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyUtilities.h:35
+// uint32_t or unsigned.
+enum WasmAddressSpace {
+  // Default address space, for pointers to unmanaged data in linear memory
----------------
Could even do this to make it clearer that this is meant to be used as an `unsigned`.


================
Comment at: llvm/test/CodeGen/WebAssembly/externref-alloca.ll:3-4
+
+;; Address space 1 is for externref values, and address space 5 is for
+;; externref-valued local variables.
+
----------------
Would it be possible to use the same address space(s) for locals and globals? In general we can use an fixed, finite number of address spaces, but I would prefer to use as few as possible so there is less to remember :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101140