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

fix(grain): Correct list lifting #2

Open
wants to merge 3 commits into
base: grain
Choose a base branch
from

Conversation

spotandjake
Copy link
Member

@spotandjake spotandjake commented Oct 23, 2024

I noticed that when you pass a list into grain it was missing one element below is the previous code for handling the lifting:

Memory.incRef(WasmI32.fromGrain(result_list_lift))
for (let mut i = WasmI32.(-)(len_list_lift, 1n); WasmI32.gtU(i, 0n); i = WasmI32.(-)(i, 1n)) {
  let base = WasmI32.(+)(base_list_lift, WasmI32.(*)(i, 4n))
  let handle_lift = {handle: WasmI32.toGrain(DataStructures.newInt32(WasmI32.load(base, 0n))),}: WebElement
  result_list_lift = [handle_lift, ...result_list_lift]
}

as you can see we initialize i from the top of the list WasmI32.(-)(len_list_lift, 1n) and iterate to the bottom of the list WasmI32.gtU(i, 0n) you may notice that we were taking two of the length because of the gt and - 1n,The bug came from the WasmI32.(-)(len_list_lift, 1n) and the solution is just to use len_list_lift. We also now need to subtract one when we are calculating base.

It may be noticed that if we switched gtU to geU we could solve this with less instructions the problem here is that when we wrap to -1 because we are working in unsigned territory -1 is greater than 0, we could switch to >= but then we would not be able to map anything outside of the range of (2^32)/2-1

@spotandjake spotandjake marked this pull request as draft October 24, 2024 00:19
@spotandjake spotandjake marked this pull request as ready for review October 24, 2024 00:38
@spotandjake spotandjake changed the title feat(grain): Correct list lifting fix(grain): Correct list lifting Oct 30, 2024
@ospencer
Copy link
Member

Let's go with just switching to geU. A list with 2^32 elements isn't possible because each element uses at least 4 bytes of memory, making the largest theoretical list something smaller than 2^30.

@spotandjake
Copy link
Member Author

spotandjake commented Dec 17, 2024

Sorry I got a little confused here (when coming back to this), geU will specifically not work because in unsigned land -1 is greater than 0, the solution would be to use the signed ge instruction however in my confusion I tried implementing this as just i != -1n which actually seems like somewhat of a better solution.

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.

2 participants