Stream: git-wasmtime

Topic: wasmtime / PR #2985 aarch64: Implement I128 Loads and Stores

view this post on Zulip Wasmtime GitHub notifications bot (Jun 14 2021 at 21:05):

afonso360 opened PR #2985 from aarch64-i128-load-store to main:

Hey, this implements loading and storing i128 values in the aarch64 backend

view this post on Zulip Wasmtime GitHub notifications bot (Jun 16 2021 at 17:10):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 16 2021 at 17:10):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 16 2021 at 17:10):

cfallin created PR review comment:

Can we add some test cases (compile-tests probably) that do basic load.i128 / store.i128 as well? At least right now, stack_store / stack_load expand to use these under the hood, but it's conceivable (especially if we try to address the codegen improvement ideas you suggest, i.e. treat sp-relative addresses differently) that this might not be true later.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 17 2021 at 14:50):

afonso360 updated PR #2985 from aarch64-i128-load-store to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 17 2021 at 14:54):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 17 2021 at 14:54):

afonso360 created PR review comment:

I'm glad I actually revisited this, because the previous code had a bug that only showed up with load.i128!

I've added the separate load.i128/store.i128 run tests, but i'm using stack_addr to get a valid address. I think this may be a problem in the future, where we may just end up optimizing that pattern(stack_addr+load) into a stack_load.

Do we have a better way of getting a R+W address?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 17 2021 at 14:55):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 17 2021 at 15:05):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 17 2021 at 15:05):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 17 2021 at 15:05):

cfallin created PR review comment:

Great, thanks for adding this (and I'm glad it found an issue)!

I think this is fine -- there isn't, afaik, a way to allocate some other memory to read/write in a run-test (there's no runtime to call to dynamically allocate, for instance). It's possible we might try to optimize stack_addr + load/store in the future (to use sp directly) but if we do that, we'll see test failures on these compile tests and then work out another solution at that point. Compile-test-only coverage is probably fine at that point, IMHO.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 17 2021 at 15:23):

cfallin merged PR #2985.

Last updated: Oct 23 2024 at 20:03 UTC