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

Sun Aug 11 06:47:29 PDT 2019

aheejin added a comment.

Thank you! We also have been floating some ideas about supporting reference types as types in another address space, but haven't actually started implementing it AFAIK. I like the general direction. Let's wait for other people's comments too.

- Are you planning to land this at some point (after adding some tests and such), or is this purely for discussions for the direction? If it's the latter, maybe I don't need to comment in the code in too much detail.. But anyway I did, so please ignore them if you are not actually planning to land this.
- Even if not to the extent to cover the whole CL, I think a few test cases still would be helpful for discussions to see how reference types can be represented in .ll files and printed in .s files.
- There are many places that add `anyref` but not `funcref`.
- Out of curiousity, is Julia planning to use reference types? If so, for which concept?
- I know it is not finished, but please clang-format if you are gonna land this later.
- Maybe you've already seen this, but if we end up landing this, we should update the linking spec <https://github.com/WebAssembly/tool-conventions/blob/master/Linking.md> in the tool convention, because we add a new relocation type.

================
Comment at: lld/wasm/WriterUtils.cpp:186
+  case ValType::FUNCREF:
+    return "func";
+  case ValType::ANYREF:
----------------
Why not `funcref`?

================
Comment at: llvm/include/llvm/BinaryFormat/WasmRelocs.def:18
 WASM_RELOC(R_WASM_TABLE_INDEX_REL_SLEB, 12)
+WASM_RELOC(R_WASM_TABLE_INDEX_LEB,      13)
----------------
I guess this is for the index to distinguish tables, not elements within the table, right? The name is not very distinguishable with `R_WASM_TABLE_INDEX_SLEB` and `R_WASM_TABLE_INDEX_I32` above, so I guess we need a new name or something

================
Comment at: llvm/include/llvm/MC/MCExpr.h:296
     VK_WASM_TYPEINDEX, // Reference to a symbol's type (signature)
+    VK_WASM_TABLEINDEX,// Reference to a table
     VK_WASM_MBREL,     // Memory address relative to memory base
----------------
Do we need this? Other symbol kinds (global, function, and events) don't have corresponding `VK_WSM_` entries here. `VK_WASM_TYPEINDEX` is left for some other reason (D58472).

================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:1233
+        Import.Kind = wasm::WASM_EXTERNAL_TABLE;
+        Import.Table.ElemType = wasm::WASM_TYPE_ANYREF;
+        Imports.push_back(Import);
----------------
Can't this be other reference types, like `FUNCREF`?

================
Comment at: llvm/lib/Object/WasmObjectFile.cpp:823
+      if (!isValidTableSymbol(Reloc.Index))
+        return make_error<GenericBinaryError>("Bad relocation event index",
+                                              object_error::parse_failed);
----------------
event -> table

================
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) {
----------------
Why should we only allow anyref?

================
Comment at: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h:51
   OPERAND_GLOBAL,
+  /// Global index.
+  OPERAND_TABLE,
----------------
Global -> Table

================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1125
+    OperandFlags = WebAssemblyII::MO_TABLE_INDEX;
+  }
+
----------------
It looks it assumes at this point we are not PIC. Maybe we can assert in `if (isPositionIndependent()) {` above that nonzero address space is not supported?

================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h:54
+      MVT::getIntegerVT(DL.getPointerSizeInBits(AS));
+  }
+
----------------
How are we gonna represent other reference types, such as `funcref` or `exnref`?

================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td:148
+let OperandType = "OPERAND_TABLE" in
+def table_op : Operand<anyref>;
+
----------------
Not sure what this means. Isn't this the type of table itself, not that of the elements?

================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:5
+             (ins table_op: $table, I32:$offset),
+             (outs), (ins table_op: $table),
+             [], !strconcat(Name, "\t$dst, ${offset}(${table})"),
----------------
- Nit: For these two lines: We usually don't seem to have a whitespace after `:` for *.td files
- Is the reason for using `offset` instead of `index` here that you plan to use named operand table methods?

================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:10
+
+defm TABLE_GET : WebAssemblyTableGet<ANYREF, "table.get", 0x25>;
+
----------------
I guess the name should be `anyref.table.get` to follow the wasm backend convention (even though we don't have separate instructions). And because "table.get" part is the constant, we only need to parameterize the prefix. This can be similar to something like [[ https://github.com/llvm/llvm-project/blob/b1a62d168f8cc639a03b0a53a7a3bd09a395069e/llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td#L26-L68 | here ]].

================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:12
+
+def : Pat<(anyref (load (WebAssemblywrapper anyref:$addr))), (TABLE_GET anyref:$addr, (CONST_I32 0))>;
----------------
I guess rules to translate instructions for non-zero indices have not been implemented here, right?

================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegisterInfo.td:47
 def EXNREF_0 : WebAssemblyReg<"%exnref.0">;

+def ANYREF_0 : WebAssemblyReg<"%anyref.0">;
----------------
Real nit: maybe we don't need a newline between reference types

================
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),
----------------
Is this gonna support existing .ll files with the previous data layout?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66035