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

[Paulo Matos via Phabricator via cfe-commits]

pmatos marked 4 inline comments as done.
pmatos added inline comments.


================
Comment at: llvm/test/CodeGen/WebAssembly/externref-tableget.ll:8
+
+define %externref @get_externref_from_table(i32 %i) {
+  %p = getelementptr [0 x %externref], [0 x %externref] addrspace (1)* @externref_table, i32 0, i32 %i
----------------
tlively wrote:
> pmatos wrote:
> > tlively wrote:
> > > It would be good to add tests with different access patterns as well like constant indices, base + offset indices, etc.
> > This is a good point and I am looking at this at the moment. Adding a few tests, reveals a couple of issues with hardcoding how a table shows up in the DAG. For example, an access with an offset reveals that the DAG is way more complicated than it needs to be. For example, for:
> > 
> > ```
> > define %externref @get_externref_from_table_with_offset(i32 %i) {
> >   %off = add nsw i32 %i, 2
> >   %p = getelementptr [0 x %externref], [0 x %externref] addrspace (1)* @externref_table, i32 0, i32 %off
> >   %ref = load %externref, %externref addrspace(1)* %p
> >   ret %externref %ref
> > }
> > ```
> > 
> > So, an access with a 2 offset from the argument index causes this dag to be generated for the access:
> > 
> > ```
> >             t2: i32 = WebAssemblyISD::ARGUMENT TargetConstant:i32<0>
> >           t12: i32 = shl t2, Constant:i32<2>
> >         t15: i32 = add t12, GlobalAddress:i32<[0 x %extern addrspace(10)*] addrspace(1)* @externref_table> 0
> >       t16: i32 = add t15, Constant:i32<8>
> >     t10: externref,ch = load<(load (s0) from %ir.p, addrspace 1)> t0, t16, undef:i32
> > ```
> > 
> > Which looks like: `(add (add (shl arg 2) table) 8)`.
> > I need to transform this back into a table load from `(add arg 2)` which seems suboptimal. I am trying to understand if it's possible to tell LLVM not to generate these address offsets for these types of accesses. It certainly seems cleaner than to reverse the computation to find the correct index to access.
> I wonder if there is a way to tell LLVM that the width of an element in the table is just 1. That looks like it would simplify this a lot, if not completely.
I managed to get that sorted in the last patch - which was to add to the datalayout string p10:8:8 and p20:8:8


================
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
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111154