afonso360 opened PR #2985 from aarch64-i128-load-store
to main
:
Hey, this implements loading and storing i128 values in the aarch64 backend
cfallin submitted PR review.
cfallin submitted PR review.
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.
afonso360 updated PR #2985 from aarch64-i128-load-store
to main
.
afonso360 submitted PR review.
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 usingstack_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 astack_load
.Do we have a better way of getting a R+W address?
afonso360 edited PR review comment.
cfallin submitted PR review.
cfallin submitted PR review.
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 usesp
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.
cfallin merged PR #2985.
Last updated: Dec 23 2024 at 13:07 UTC