Skip to content

Conversation

@sbc100
Copy link
Member

@sbc100 sbc100 commented Jul 11, 2024

This edge case make the lowering a little more tricky.

This edge case make the lowering a little more tricky.
@sbc100 sbc100 requested review from kripken and tlively July 11, 2024 22:10
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this is doing "if we get i32(-1), return i64(-1), else return u64(result)"? Can we not do "s64(result)"?

  1. If the result is -1 then sign-extending it works properly.
  2. If the result is the number of pages then sign-extending may be wrong if the sign bit was set. But can it be set? That would imply we allocated 2^31 pages of size 2^16 which is 47 bits... surely that is over the limit? 😄

@sbc100 sbc100 force-pushed the memory64_lowering branch from e937766 to 0215780 Compare July 11, 2024 22:26
@sbc100
Copy link
Member Author

sbc100 commented Jul 11, 2024

IIUC this is doing "if we get i32(-1), return i64(-1), else return u64(result)"? Can we not do "s64(result)"?

I guess that could work?

  1. If the result is -1 then sign-extending it works properly.
  2. If the result is the number of pages then sign-extending may be wrong if the sign bit was set. But can it be set? That would imply we allocated 2^31 pages of size 2^16 which is 47 bits... surely that is over the limit? 😄

If we ever get variable sized pages I guess that would break?

@sbc100
Copy link
Member Author

sbc100 commented Jul 11, 2024

IIUC this is doing "if we get i32(-1), return i64(-1), else return u64(result)"? Can we not do "s64(result)"?

I guess that could work?

  1. If the result is -1 then sign-extending it works properly.
  2. If the result is the number of pages then sign-extending may be wrong if the sign bit was set. But can it be set? That would imply we allocated 2^31 pages of size 2^16 which is 47 bits... surely that is over the limit? 😄

If we ever get variable sized pages I guess that would break?

Should we keep it simple and use signed extension in this case rather than this complex code block? At the risk of breaking if we ever gets byte-sized pages?

@kripken
Copy link
Member

kripken commented Jul 11, 2024

Oh, good point, yeah, if we use variable-sized pages then we might have 1-byte pages... In which case 2^31 is not that much at all!

Let's keep the code you've already written, given that. It's not that complicated and it is future-proof.

@kripken
Copy link
Member

kripken commented Jul 11, 2024

Wait, how would byte-sized pages work? It seems like memory.grow would need to start returning an i64..?

@sbc100 sbc100 enabled auto-merge (squash) July 11, 2024 22:42
@sbc100
Copy link
Member Author

sbc100 commented Jul 11, 2024

Wait, how would byte-sized pages work? It seems like memory.grow would need to start returning an i64..?

Good point. We should probably raise this on the variable page size repo.

@sbc100
Copy link
Member Author

sbc100 commented Jul 11, 2024

Wait, how would byte-sized pages work? It seems like memory.grow would need to start returning an i64..?

Its only the special -1 value (0xffffffff) that is ear marked as an error... so this would just mean that under wasm32 you would only be able to grow your single-byte page-size memory to MAX_UINT32-1 rather than MAX_UINT32. That doesn't seem so bad does it?

@kripken
Copy link
Member

kripken commented Jul 11, 2024

Noted in an upstream issue: WebAssembly/custom-page-sizes#28

Yeah, maybe the idea is that when the page size is small you only use wasm32? But it seems like in general there are use cases that could involve wasm64.

@sbc100 sbc100 merged commit 22c28bd into main Jul 11, 2024
@sbc100 sbc100 deleted the memory64_lowering branch July 11, 2024 23:36
@sbc100
Copy link
Member Author

sbc100 commented Jul 11, 2024

Noted in an upstream issue: WebAssembly/custom-page-sizes#28

Yeah, maybe the idea is that when the page size is small you only use wasm32? But it seems like in general there are use cases that could involve wasm64.

With a 64-bit memory, memory.grow takes i64 and returns i64

@kripken
Copy link
Member

kripken commented Jul 11, 2024

Oh, right, the issue is that this is for the lowering pass where the input is "unnaturally" an i32. I'll close that issue then.

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

Successfully merging this pull request may close these issues.

4 participants