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

Consolidate address calculations for atomics #3143

Merged
merged 3 commits into from
Aug 4, 2021

Conversation

alexcrichton
Copy link
Member

This commit consolidates all calcuations of guest addresses into one
prepare_addr function. This notably remove the atomics-specifics paths
as well as the prepare_load function (now renamed to prepare_addr
and folded into get_heap_addr).

The goal of this commit is to simplify how addresses are managed in the
code generator for atomics to use all the shared infrastrucutre of other
loads/stores as well. This additionally fixes #3132 via the use of
heap_addr in clif for all operations.

I also added a number of tests for loads/stores with varying alignments.
Originally I was going to allow loads/stores to not be aligned since
that's what the current formal specification says, but the overview of
the threads proposal disagrees with the formal specification, so I
figured I'd leave it as-is but adding tests probably doesn't hurt.

Closes #3132

This commit consolidates all calcuations of guest addresses into one
`prepare_addr` function. This notably remove the atomics-specifics paths
as well as the `prepare_load` function (now renamed to `prepare_addr`
and folded into `get_heap_addr`).

The goal of this commit is to simplify how addresses are managed in the
code generator for atomics to use all the shared infrastrucutre of other
loads/stores as well. This additionally fixes bytecodealliance#3132 via the use of
`heap_addr` in clif for all operations.

I also added a number of tests for loads/stores with varying alignments.
Originally I was going to allow loads/stores to not be aligned since
that's what the current formal specification says, but the overview of
the threads proposal disagrees with the formal specification, so I
figured I'd leave it as-is but adding tests probably doesn't hurt.

Closes bytecodealliance#3132
@alexcrichton alexcrichton requested a review from cfallin August 4, 2021 18:46
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:wasm labels Aug 4, 2021
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

LGTM; thanks very much for this, it's a lot simpler and cleaner than what we had before!

Very good catch on the wasm linear address vs. native pointer documentation and signature fix.

It's not immediately apparent to me what's going on with the aarch64 failure (the misalignment check looks like it should be platform-agnostic); happy to help debug if you're not sure either.

@alexcrichton
Copy link
Member Author

Looks like #3144 is what this was running into. The trap showing up was different for emulation vs non-emulation (due to configurations around guard pages and linear memory). I'm not sure which trap should be showing up so I've modified the tests to only have one case of the trap possible, instead of two.

@alexcrichton
Copy link
Member Author

Hm well actually thinking about it more, the in-progress spec does specify that alignment checks happen first, and I think that's the most reasonable thing to do, so I'm going to go ahead and implement that. The codegen is inefficient in that it generates an iadd purely for the alignment check which probably overlaps with the later iadd for the the lowering of heap_addr or adding in the offset again, but this should at least be more correct than before and presumably optimizations can always come later too.

@cfallin
Copy link
Member

cfallin commented Aug 4, 2021

Yeah, GVN may actually combine the adds; if it doesn't, we might be able to do something special for this later.

@alexcrichton alexcrichton merged commit 85f16f4 into bytecodealliance:main Aug 4, 2021
@alexcrichton alexcrichton deleted the update-sthreads branch August 4, 2021 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:wasm cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

threads: Addresses for atomic operations silently wrap instead of using an effective 33-bit index
2 participants