This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Implementation of table.get/set for reftypes in LLVM IR
ClosedPublic

Authored by pmatos on Oct 5 2021, 7:26 AM.

Details

Summary

This change implements new DAG nodes TABLE_GET/TABLE_SET, and lowering
methods for load and stores of reference types from IR arrays. These
global LLVM IR arrays represent tables at the Wasm level.

Diff Detail

Event Timeline

pmatos created this revision.Oct 5 2021, 7:26 AM
pmatos requested review of this revision.Oct 5 2021, 7:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2021, 7:26 AM
pmatos updated this revision to Diff 377476.Oct 6 2021, 2:11 AM

Updating over current HEAD to allow it to build in buildbot.

It's very cool to see this all coming together!

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
91

I think the MVT::Other can be rolled into the existing loop. It would also be good to add a comment here about why MVT::Other comes up here.

821

I expect that you'll have to do something here as well.

1429

This can just be a cast if we can assume that it will succeed. (Or maybe we should be checking for success here?)

1471

It would be helpful to add a comment here about what DAG patterns we are expecting to find.

1476–1478

Is there still an ADD when the index is a constant zero?

1487

It seems strange to use a dyn_cast for this (and I'm surprised that works at all!)

1551–1575

It would be good to deduplicate this code with the store case as much as possible.

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
48

Would be good to fix before landing if possible!

llvm/test/CodeGen/WebAssembly/externref-tableget.ll
8

It would be good to add tests with different access patterns as well like constant indices, base + offset indices, etc.

20

Does this need more information to say it is a table?

llvm/test/CodeGen/WebAssembly/funcref-table_call.ll
25–27

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.

pmatos added inline comments.Oct 7 2021, 3:22 AM
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
91

Makes sense.

821

I am confused. Is this comment maybe related to D111227 instead?

1429

Right, the will always succeed because of the IsWebAssemblyGlobal condition. Moving to cast.

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
48

Oh, yes, I forgot this. Thanks for pointing it out.

tlively added inline comments.Oct 7 2021, 1:04 PM
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
821

Oh, actually I think I was confused as well. Disregard this.

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
77

Do you have ideas for this simplification as well?

pmatos added inline comments.Oct 8 2021, 2:41 AM
llvm/test/CodeGen/WebAssembly/externref-tableget.ll
8

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.

tlively added inline comments.Oct 8 2021, 12:13 PM
llvm/test/CodeGen/WebAssembly/externref-tableget.ll
8

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.

pmatos updated this revision to Diff 379917.Oct 14 2021, 9:56 PM

Simplified a lot of code that required some further fixes and de-duplication.

Most importantly, I added many more testcases to ensure different addressing modes are supported.
The way to tell LLVM that externref and funcref pointers should not have complex addressing DAGS with shift/add/muls,
was to add that information to the DataLayout string (p10:8:8-p20:8:8). This forced changes in many testfiles.

This looks now ready for another round of reviewing.

pmatos marked 4 inline comments as done.Oct 14 2021, 10:03 PM
pmatos added inline comments.
llvm/test/CodeGen/WebAssembly/externref-tableget.ll
8

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

llvm/test/CodeGen/WebAssembly/funcref-table_call.ll
25–27

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

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

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1451

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.

llvm/test/CodeGen/WebAssembly/funcref-table_call.ll
25–27

Ah right, thanks!

pmatos updated this revision to Diff 380337.Oct 18 2021, 4:04 AM

Use reference to pointer instead of pointer to pointer as @tlively suggested.

tlively accepted this revision.Oct 19 2021, 6:11 PM

Nice! Just a couple nits but I think this is good to go.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1474–1475

It would be nice to switch the if arms around so the else corresponds with the null case.

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
80–83

It would be good to explicitly check the externref address space as well so we can error out if we get an unexpected address space.

This revision is now accepted and ready to land.Oct 19 2021, 6:11 PM

Nice! Just a couple nits but I think this is good to go.

Perfect, thanks for the comments. Fixed nits and landed.