alexcrichton opened PR #3134 from current-length-usize
to main
:
This commit changes the definition of
VMMemoryDefinition::current_length
tousize
from its previous
definition ofu32
. This is a pretty impactful change because it also
changes the cranelift semantics of "dynamic" heaps where the bound
global value specifier must now match the pointer type for the platform
rather than the index type for the heap.The motivation for this change is that the
current_length
field (or
bound for the heap) is intended to reflect the current size of the heap.
This is bound byusize
on the host platform rather thanu32
oru64
. The previous choice ofu32
couldn't represent a 4GB memory
because we couldn't put a number representing 4GB into the
current_length
field. By usingusize
, which reflects the host's
memory allocation, this should better reflect the size of the heap and
allows Wasmtime to support a full 4GB heap for a wasm program (instead
of 4GB minus one page).This commit also updates the legalization of the
heap_addr
clif
instruction to appropriately cast the address to the platform's pointer
type, handling bounds checks along the way. The practical impact for
today's targets is that auextend
is happening sooner than it happened
before, but otherwise there is no intended impact of this change. In the
future when 64-bit memories are supported there will likely need to be
fancier logic which handles offsets a bit differently (especially in the
case of a 64-bit memory on a 32-bit host).The clif
filetest
changes should show the differences in codegen, and
the Wasmtime changes are largely removing casts here and there.Closes #3022
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
alexcrichton updated PR #3134 from current-length-usize
to main
.
pchickey submitted PR review.
alexcrichton requested cfallin for a review on PR #3134.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
We should note that this also asserts on 32/32 and 64/64 cases; I don't think we need to do anything here in either case. I think this is fine for now -- better to assert out if we haven't thought carefully about it and if it's a theoretical need, especially in address computation logic -- but perhaps add a comment to this effect?
cfallin created PR review comment:
Not strictly in-scope (sorry!) and I see you've just moved this code from below, but since we're looking at it now: we should have a method on the DFG for this -- maybe something like
add_value_label_alias()
? If not, at least a comment to the effect of "Add debug value-label alias so that debuginfo can name the extended value as the address".
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Oh hm wouldn't that be covered by the
offset_ty == addr_ty
check above?I do want to make sure that unhandled cases are documented here for future readers, but I also figured that the matching pointer/offset cases were handled above.
cfallin submitted PR review.
cfallin created PR review comment:
Oh goodness -- I should, as a rule, discard any code review I do on Monday morning before the coffee is complete. Yes, I missed that, sorry!
alexcrichton updated PR #3134 from current-length-usize
to main
.
alexcrichton merged PR #3134.
Last updated: Dec 23 2024 at 12:05 UTC