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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
It's very cool to see this all coming together!
| llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp | ||
|---|---|---|
| 91–93 | 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. | |
| 825 | I expect that you'll have to do something here as well. | |
| 1433 | This can just be a cast if we can assume that it will succeed. (Or maybe we should be checking for success here?) | |
| 1551 | It would be helpful to add a comment here about what DAG patterns we are expecting to find. | |
| 1556–1558 | Is there still an ADD when the index is a constant zero? | |
| 1567 | It seems strange to use a dyn_cast for this (and I'm surprised that works at all!) | |
| 1611–1635 | 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 | ||
| 9 | It would be good to add tests with different access patterns as well like constant indices, base + offset indices, etc. | |
| 21 | Does this need more information to say it is a table? | |
| llvm/test/CodeGen/WebAssembly/funcref-table_call.ll | ||
| 26–28 | 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. | |
| llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp | ||
|---|---|---|
| 91–93 | Makes sense. | |
| 825 | I am confused. Is this comment maybe related to D111227 instead? | |
| 1433 | 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. | |
| llvm/test/CodeGen/WebAssembly/externref-tableget.ll | ||
|---|---|---|
| 9 | 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:i32Which looks like: (add (add (shl arg 2) table) 8). | |
| llvm/test/CodeGen/WebAssembly/externref-tableget.ll | ||
|---|---|---|
| 9 | 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. | |
Comment Actions
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.
| llvm/test/CodeGen/WebAssembly/externref-tableget.ll | ||
|---|---|---|
| 9 | 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 | ||
| 26–28 | 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 | |
Comment Actions
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 | ||
|---|---|---|
| 1455 | 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 | ||
| 26–28 | Ah right, thanks! | |
Comment Actions
Nice! Just a couple nits but I think this is good to go.
| llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp | ||
|---|---|---|
| 1478–1479 | It would be nice to switch the if arms around so the else corresponds with the null case. | |
| llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp | ||
| 72–75 | 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. | |
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.