With reference types, tables can have non-zero table numbers. This
commit adds support for element sections against these tables.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/MC/WasmObjectWriter.cpp | ||
---|---|---|
937 | inline comments from @sbc100, from https://reviews.llvm.org/D92321?id=323966#inline-917851:
|
llvm/lib/MC/WasmObjectWriter.cpp | ||
---|---|---|
937 | Given that we need the logic in 4 different files, it seemed like defining the mask in a central place would be a good idea. You're right though that as it was, you could get confused and think that (WASM_ELEM_SEGMENT_HAS_TABLE_NUMBER & WASM_ELEMENT_SEGMENT_HAS_ELEM_KIND) == 0. I changed the name therefore to WASM_ELEM_SEGMENT_MASK_HAS_ELEM_KIND; wdyt? There's no accounting for taste, obviously, and if you have a strong preference here, I am very happy to adapt. I think all the other inline comments from D92321 have been addressed in this patch, fwiw. |
lgtm!
llvm/include/llvm/BinaryFormat/Wasm.h | ||
---|---|---|
316 | Do you think this should outside the enum perhaps? | |
llvm/lib/MC/WasmObjectWriter.cpp | ||
919 | It looks like we pass IndirectFunctionTable just to get the table nunber. Would it make sense to instead change the signature of this functions to: void WasmObjectWriter::writeElemSection(uint32_t TableNumber, ArrayRef<uint32_t> TableElems) And then it can be re-used later once we support different tables? Or maybe that be part of the followup? lgtm which ever way you think makes sense. |
llvm/lib/MC/WasmObjectWriter.cpp | ||
---|---|---|
919 | Yeah that's right. Would work fine to pass a table number, only except that if TableElems is empty, it could be that there's no function table symbol. Once we start having different / other tables, we may want to split things; there may be multiple segments against different tables, for example. If OK with you, will reserve any needed refactors for a followup. |
llvm/include/llvm/BinaryFormat/Wasm.h | ||
---|---|---|
316 | fixed when pushed as const unsigned WASM_ELEM_SEGMENT_MASK_HAS_ELEM_KIND = 0x3; |
Do you think this should outside the enum perhaps?