[PATCH] D66035: [WebAssembly] WIP: Add support for reference types

[Valentin Churavy via Phabricator via cfe-commits]

vchuravy marked 4 inline comments as done.
vchuravy added a comment.

Rebased onto current master and added an initial test. (I will start adding more as I start integrating this with the rest of the toolchain)



================
Comment at: llvm/lib/Object/WasmObjectFile.cpp:998
+    if (Tables.back().ElemType != wasm::WASM_TYPE_FUNCREF &&
+        // TODO: Only allow anyref here when reference-types is enabled?
+        Tables.back().ElemType != wasm::WASM_TYPE_ANYREF) {
----------------
aheejin wrote:
> Why should we only allow anyref?
Not sure what Keno meant by this comment, but it might be to check that `reference-types` is enabled before allowing it as a element type.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssembly.h:88
+  MAX_CUSTOM_ADDRESS = 255,
+  ANYREF_ADDRESS = 256, // Address space for anyref
+};
----------------
tlively wrote:
> I assume other reference types will get their own address spaces so we can differentiate them in IR. Would it then make sense to put the table itself in another address space? We should also think about how best to partition the address space space looking forward to when we will have multiple memories, multiple tables, and multiple function references or reference type imports.
Yes. `funcref` should get its own address space but I am not sure about its usage yet.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h:54
+      MVT::getIntegerVT(DL.getPointerSizeInBits(AS));
+  }
+
----------------
aheejin wrote:
> How are we gonna represent other reference types, such as `funcref` or `exnref`?
The question is which of these types need representing. As far as I can tell `exnref` and is produced by LLVM already and `funcref` is primarily used for function tables in modules and isn't necessarily a frontend facing time. Albeit nothing in the spec says that it couldn't be. 

If funcref would need a frontend facing representation it would need a new AS.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:113
+                        TT.isArch64Bit() ? "e-m:e-p:64:64-i64:64-n32:64-S128-ni:1"
+                                         : "e-m:e-p:32:32-i64:64-n32:64-S128-ni:1",
                         TT, CPU, FS, Options, getEffectiveRelocModel(RM, TT),
----------------
aheejin wrote:
> Is this gonna support existing .ll files with the previous data layout?
I don't see why it shouldn't without activating `-mattr=+reference-types` new .ll files won't compile,
but old one should.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66035