[multibyte] Add multibyte array load instructions.#8504
[multibyte] Add multibyte array load instructions.#8504brendandahl wants to merge 1 commit intoWebAssembly:mainfrom
Conversation
Prototype implementation of the new multibyte array proposal that reuses the existing memory instructions. https://github.com/WebAssembly/multibyte-array-access/blob/944d79230b336442b1f0b3978da1908d54b9f230/proposals/multibyte-array-access/Overview.md
brendandahl
requested review from
stevenfontanella
and removed request for
a team
| if (curr->bytes == 1) sval = int64_t(int8_t(sval)); | ||
| else if (curr->bytes == 2) sval = int64_t(int16_t(sval)); | ||
| else if (curr->bytes == 4) sval = int64_t(int32_t(sval)); |
There was a problem hiding this comment.
Can we use C++ style casts or bit operators instead of C-style casts? e.g. sval & 0xFF
| if (curr->ref->type.isNull()) { | ||
| parent.trap = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
I think we can use
if (trapOnNull(curr->ref)) {
return;
}which also handles the unreachable case. I guess the below statement setting implicitTrap is still needed because OOB access is always possible?
There was a problem hiding this comment.
It would be good to add a comment about the possibility of OOB access here.
| } | ||
| } | ||
|
|
||
| void ArrayLoad::finalize() { |
There was a problem hiding this comment.
I'm confused on this, doesn't ArrayLoad have a return value based on the value of bytes and signed?
There was a problem hiding this comment.
I think the issue is bytes and signed is not always enough to differentiate integer and floating point loads, so we're already depending on the type be set correctly to tell us what instruction this is (and we wouldn't know how to update it even if we wanted to.)
| visit(curr->index) + visit(curr->value); | ||
| } | ||
| CostType visitArrayLoad(ArrayLoad* curr) { | ||
| return 1 + nullCheckCost(curr->ref) + visit(curr->ref) + visit(curr->index); |
There was a problem hiding this comment.
What's the 1 for here? Can we name this constant?
There was a problem hiding this comment.
We use lots of unnamed constants for base costs, so I think considering this separately would be fine.
stevenfontanella
requested review from
a team and
tlively
and removed request for
a team
|
(Re-add binaryen reviewers for a second look) |
| visit(curr->index) + visit(curr->value); | ||
| } | ||
| CostType visitArrayLoad(ArrayLoad* curr) { | ||
| return 1 + nullCheckCost(curr->ref) + visit(curr->ref) + visit(curr->index); |
There was a problem hiding this comment.
We use lots of unnamed constants for base costs, so I think considering this separately would be fine.
| if (curr->ref->type.isNull()) { | ||
| parent.trap = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
It would be good to add a comment about the possibility of OOB access here.
| auto shifts = Literal(int32_t(32 - field->getByteSize() * 8)); | ||
| unsigned bits = resultType.getByteSize() == 8 ? 64 : 32; | ||
| if (bits <= bytes * 8) { | ||
| return; // No need to sign-extend for full size |
There was a problem hiding this comment.
We generally avoid inline comments.
| return; // No need to sign-extend for full size | |
| // No need to sign-extend for full size. | |
| return; |
| } | ||
| } | ||
|
|
||
| void ArrayLoad::finalize() { |
There was a problem hiding this comment.
I think the issue is bytes and signed is not always enough to differentiate integer and floating point loads, so we're already depending on the type be set correctly to tell us what instruction this is (and we wouldn't know how to update it even if we wanted to.)
| if (curr->bytes == 1) sval = int32_t(int8_t(sval)); | ||
| else if (curr->bytes == 2) sval = int32_t(int16_t(sval)); |
There was a problem hiding this comment.
We always use brackets around if bodies, even for one-liners. (Here and below.)
3 participants
Prototype implementation of the new multibyte array proposal that reuses the existing memory instructions.
https://github.com/WebAssembly/multibyte-array-access/blob/944d79230b336442b1f0b3978da1908d54b9f230/proposals/multibyte-array-access/Overview.md