[PATCH] D111154: [WebAssembly] Implementation of table.get/set for reftypes in LLVM IR

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

This is looking good! I'll take a more thorough pass through tomorrow so we can get this landed.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1455
+                                                      const SDValue &Base,
+                                                      GlobalAddressSDNode **GA,
+                                                      SDValue &Idx) const {
----------------
Would it make sense to use `GlobalAddressSDNode *&GA` here to match using a reference for the `Idx` out-param? That would slightly simplify the code below as well.


================
Comment at: llvm/test/CodeGen/WebAssembly/funcref-table_call.ll:25-27
+; CHECK-NEXT: i32.const       0
+; CHECK-NEXT: ref.null        func
+; CHECK-NEXT: table.set       __funcref_call_table
----------------
pmatos wrote:
> tlively wrote:
> > Do you think it would be a good idea to skip setting this table slot back to null? I know we discussed this before, but I don't remember what the pros and cons were.
> So the reason to do this was that if we leave the funcref in the slot, this could create hidden GC roots. Comes from this comment by @wingo : https://reviews.llvm.org/D95425#inline-929792
Ah right, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111154