Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for 64-bit call_indirect or full table64 support #46

Closed
Tracked by #43
sbc100 opened this issue Feb 15, 2024 · 19 comments
Closed
Tracked by #43

Adding support for 64-bit call_indirect or full table64 support #46

sbc100 opened this issue Feb 15, 2024 · 19 comments

Comments

@sbc100
Copy link
Member

sbc100 commented Feb 15, 2024

When targeting wasm64 today llvm currently truncates the i64 pointer operand to call_indirect since call_indirect currently requires a i32.

Should we add support for an i64 operand to call_indirect as part of this proposal?

If so, should we do this by adding the concept of 64-bit-indexed table?

There have been some discussion of this already in both #43 and in #10 since I thought it worth creating a separate issue.

@sbc100 sbc100 changed the title Should we include support for 64-bit call_indirect or full table64 support in this proposal Adding support for 64-bit call_indirect or full table64 support Feb 15, 2024
@sbc100
Copy link
Member Author

sbc100 commented Feb 15, 2024

My understanding is that this would not but much work from the implementation side. @backes and @eqrion perhaps you could confirm?

(To be clear this would not involve implementations having to increase the size of the tables they support)

@sunfishcode
Copy link
Member

I don't see a strong motivation for this. Looking at #43 and #10, the concrete motivations seem to be:

  • Avoiding conversion instructions before call_indirect instructions in typical compiled C code, which is theorized to be a code-size win, however the theoretical impact has not been calculated.
  • A compiler that deliberately violates the Wasm spec and omits sandboxing would be helped, however this is not a priority for Wasm.

@eqrion
Copy link
Contributor

eqrion commented Feb 16, 2024

My understanding is that this would not but much work from the implementation side. @backes and @eqrion perhaps you could confirm?

(To be clear this would not involve implementations having to increase the size of the tables they support)

It would be a 'fair' amount of work. We'd have to update our codegen for call_indirect bounds checking to perform the truncation/overflow check, including on 32-bit systems where the 64-bit pointer is two registers. Nothing we've not done before, but also not just a couple days of work.

@rossberg
Copy link
Member

IIRC, the problem (reported by @matthias-blume at the time) was that implementing the right behaviour for invalid function pointers when "safely" compiling something like C required manual bounds checks in user code, since simply truncating from 64 to 32 bit before call_indirect might accidentally result in a successful call through an invalid pointer.

There also is the question of coherence of Wasm itself. We generally tried to keep memories and tables as similar as possible, especially after tables were repurposed to a general storage for references with the GC proposal, tables now being to reference types what memory is to numeric types.

@backes
Copy link
Member

backes commented Feb 19, 2024

My understanding is that this would not but much work from the implementation side. @backes and @eqrion perhaps you could confirm?

(To be clear this would not involve implementations having to increase the size of the tables they support)

Implementing explicit bounds checking of i64 values against 32-bit values (e.g. for memory accesses on 32-bit platforms) didn't turn out to be particularly hard in V8. I guess we could reuse most of that for 64-bit table indexing.
I think "a few days of work" is the right bucket for this.

@eqrion
Copy link
Contributor

eqrion commented Feb 21, 2024

So just to make sure I understand what's being proposed, this would be a general '(table i64)' feature even if we maintain a <2^32 implementation limit on their size?

So we'd need to:

  1. Allow the limits of a tabletype to specify an i64 index type
  2. Extend text format parsing of tables to allow specifying index type
  3. Extend JS-API table constructors to take index type
  4. Extend JS-API get/set methods to take i64 values? (might be superseded by an implementation limit)
  5. Update the following instructions to take i32 or i64, depending on the type:
    - table.get/set/size
    - table.grow?
    - call_indirect

I think this could be a reasonable feature, and the example of correct compilation that Rossberg mentions seems compelling.

@matthias-blume
Copy link

matthias-blume commented Feb 21, 2024 via email

@sbc100
Copy link
Member Author

sbc100 commented Mar 25, 2024

I've put some time on tomorrow's CG meeting agenda to discuss this issue.

@sbc100
Copy link
Member Author

sbc100 commented Mar 26, 2024

As per today's CG discussion lets have an informal poll here by voting thumbs up or thumbs down on OP of this issue.

@sbc100
Copy link
Member Author

sbc100 commented Apr 13, 2024

Looks like we have 8 to 1 for adding this to memory64 proposal. @conrad-watt do you think we need to take this back to the CG or can we simply move ahead with this? I'm hoping we can simply start work on it immediately (perhaps we can do both.. i.e. start work and have a vote ASAP).

@conrad-watt
Copy link
Contributor

Given the late stage of the proposal, this change should be confirmed with a pre-announced vote in the CG.

Given the straw poll here, I think it's ok for work to be started in anticipation of the vote result, but just be aware that if the vote goes a surprising way there may be the potential for wasted work!

@rossberg
Copy link
Member

Given the late stage of the proposal, this change should be confirmed with a pre-announced vote in the CG.

Just want to note that this isn't a change, merely an addition, so champion's discretion could suffice? Technically, the only required votes are the stage votes.

@conrad-watt
Copy link
Contributor

Technically, the only required votes are the stage votes.

There's no hard and fast rule on how much a proposal can be changed at phase 3, but assuming we're going with full table64 support we're essentially skipping a (small) feature-sized group of instructions past the regular phase process to phase 3. We should take extra care to make sure consensus on this decision is well-recorded, especially since the proposal has otherwise stayed unchanged for quite a while and there's no associated subgroup where regular discussions are happening.

It's not world-ending either way, but IMO it's better to take the extra meeting slot to be very deliberate about this and minimize surprise for the ecosystem.

@conrad-watt
Copy link
Contributor

conrad-watt commented Apr 22, 2024

@sbc100 would you be ok with us slotting a brief confirmatory vote into tomorrow's meeting, or do you want this to settle for a little longer? (edit: see PR)

conrad-watt added a commit to WebAssembly/meetings that referenced this issue Apr 22, 2024
subject to a positive response here WebAssembly/memory64#46 (comment)
dschuff pushed a commit to WebAssembly/meetings that referenced this issue Apr 22, 2024
@sbc100
Copy link
Member Author

sbc100 commented Apr 23, 2024

The poll on today's CG meeting was positive (all votes we in favor or neutral) so it looks like we will be going ahead with this.

@sbc100
Copy link
Member Author

sbc100 commented Apr 23, 2024

  • Extend JS-API get/set methods to take i64 values? (might be superseded by an implementation limit)

This issue likely requires come consideration. In JavaScript i64 pointers tend to be represented as BigInt values and by extension it would make sense to allow these pointers to be used as arguments to the JavaScript .set and .get methods on tables.

However, in emscripten today it actually convert pointers to JavaScript Numbers (i.e. 53bit numbers) so for the emscripten POV it would be great if the .get and .set methods for 64-bit tables could either either Number or BigInt as arguments.

@conrad-watt
Copy link
Contributor

What is currently done for 64 bit memories in the JS API? I vaguely remember we previously discussed a 2^53 JS implementation limit on memory size, to ensure that regular JS numbers could continue to be used as indices. If we followed through on this idea for memories, we could do the same for tables.

@sbc100
Copy link
Member Author

sbc100 commented Apr 23, 2024

What is currently done for 64 bit memories in the JS API? I vaguely remember we previously discussed a 2^53 JS implementation limit on memory size, to ensure that regular JS numbers could continue to be used as indices. If we followed through on this idea for memories, we could do the same for tables.

Memory access is done via JavaScript typed arrays which can be indexed by both Number and BigInt. By analogy I suppose that .get and .set should also accept both.

@sbc100
Copy link
Member Author

sbc100 commented Apr 24, 2024

Closing this issue for now. Implementation will be tracked in #51.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants