Winch: implement rmw add ops #9990
Conversation
MarinPostma
requested review from
cfallin and
dicej
and removed request for
a team
Subscribe to Label Action
This issue or pull request has been labeled: "winch"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
|
I can take this review. |
saulecabrera
requested review from
saulecabrera
and removed request for
cfallin and
dicej
MarinPostma
force-pushed
the
winch-x64-rmw-add
branch
2 times, most recently
from
028a95e to
a5e07de
Compare
winch/codegen/src/visitor.rs
Outdated
| &arg, | ||
| RmwOp::Add, | ||
| OperandSize::S16, | ||
| Some(ExtendKind::I32Extend16S), |
There was a problem hiding this comment.
The extension kinds used in this case are signed, however the spec states zero extension instead. I believe we need to augment ExtendKind to account for the missing unsigned extensions.
There was a problem hiding this comment.
that's because https://github.com/bytecodealliance/wasmtime/blob/main/winch/codegen/src/isa/x64/asm.rs#L395-L401 takes ExtendKind, even though movzx zero-extend. I can do that in a followup, if that's ok? i'll add variants for unsigned conversions, and update any affected code.
There was a problem hiding this comment.
I've extended ExtendKind here #10012 to include unsigned extends.
winch/codegen/src/codegen/mod.rs
Outdated
| @@ -1295,6 +1295,31 @@ where | |||
|
|
|||
| Ok(()) | |||
| } | |||
|
|
|||
| pub(crate) fn atomic_rmw( | |||
There was a problem hiding this comment.
| pub(crate) fn atomic_rmw( | |
| pub(crate) fn emit_atomic_rmw( |
MarinPostma
force-pushed
the
winch-x64-rmw-add
branch
from
a5556ed to
6507f0c
Compare
winch/codegen/src/codegen/mod.rs
Outdated
| extend: Option<ExtendKind>, | ||
| ) -> Result<()> { | ||
| let operand = self.context.pop_to_reg(self.masm, None).unwrap(); | ||
| if let Some(addr) = self.emit_compute_heap_address(arg, size)? { |
There was a problem hiding this comment.
Just a note that this will require a rebase once #9987 lands and use emit_compute_heap_address_align_checked
|
@saulecabrera I'll do all the rebases once #9987 is in |
MarinPostma
force-pushed
the
winch-x64-rmw-add
branch
from
6507f0c to
794a14c
Compare
MarinPostma
force-pushed
the
winch-x64-rmw-add
branch
from
ea19990 to
53d2d25
Compare
|
@saulecabrera rebase is done. I have also added a check to ensure that only valid extends are passed to |
winch/codegen/src/isa/x64/masm.rs
Outdated
| // It is only necessary to zero-extend when the operand is less than 32bits. | ||
| // x64 automatically zero-extend 32bits to 64bit. | ||
| Some( | ||
| extend @ (ExtendKind::I32Extend8S | ||
| | ExtendKind::I64Extend8S | ||
| | ExtendKind::I64Extend16S | ||
| | ExtendKind::I32Extend16S), | ||
| ) => { |
There was a problem hiding this comment.
I believe the check here needs updating? The code is emitting a zero extend but matching against a signed extend? Also I believe that to: (i) reduce duplication and (ii) avoid affecting compile time performance, we can entirely remove this check, since Cranelift's emission layer already takes care of it https://github.com/bytecodealliance/wasmtime/blob/main/cranelift/codegen/src/isa/x64/inst/emit.rs#L973.
There was a problem hiding this comment.
good catch i'll remove it
MarinPostma
force-pushed
the
winch-x64-rmw-add
branch
from
5ef91f7 to
402d54b
Compare
impl read-modify-write add for winch:
i32.atomic.rmw8.add_ui32.atomic.rmw16.add_ui32.atomic.rmw.addi64.atomic.rmw8.add_ui64.atomic.rmw16.add_ui64.atomic.rmw32.add_ui64.atomic.rmw.add#9734