Winch aarch64 extend, wrap, popcnt, promote & demote#9114
Winch aarch64 extend, wrap, popcnt, promote & demote#9114saulecabrera merged 14 commits intobytecodealliance:mainfrom
Conversation
5a2afc0 to
638b4a5
Compare
|
I can help reviewing this one. |
winch/codegen/src/isa/aarch64/asm.rs
Outdated
| let (signed, from_bits, to_bits) = match kind { | ||
| I64ExtendI32S => (true, 32, 64), | ||
| I64ExtendI32U => (false, 32, 64), | ||
| I32Extend8S => (true, 8, 32), | ||
| I32Extend16S => (true, 16, 32), | ||
| I64Extend8S => (true, 8, 64), | ||
| I64Extend16S => (true, 16, 64), | ||
| I64Extend32S => (true, 32, 64), | ||
| }; |
There was a problem hiding this comment.
Could we add some methods to the ExtendKind implementation instead? Something like:
impl ExtendKind {
fn signed(&self) -> bool { ... }
fn from_bits(&self) -> u8 { ... }
fn to_bits(&self) -> u8 { ... }
}| todo!() | ||
| fn popcnt(&mut self, context: &mut CodeGenContext, size: OperandSize) { | ||
| let src = context.pop_to_reg(self, None); | ||
| let tmp = context.reg_for_class(RegClass::Float, self); |
There was a problem hiding this comment.
To reduce register pressure, could we use the scratch float register here?
There was a problem hiding this comment.
In the case of aarch64 I believe that the fallback test is redundant i.e., since we are not generating an alternative sequence of instructions like in x64.
| OperandSize::S8 => {} | ||
| OperandSize::S16 => self.asm.addp_rrr(tmp, tmp, tmp, VectorSize::Size8x8), |
There was a problem hiding this comment.
If I'm not wrong, Wasm only defines {i32, i64}_popcnt so I believe we only need to handle the S32 | S64 case and handle all the other operand sizes with unreachable!() instead of unimplemented, which also means that we can probably drop the addp_rrr implementation.
There was a problem hiding this comment.
You're right, I used lower.isle as a reference but CLIF is different from WASM.
handle all the other operand sizes with
unreachable!()instead ofunimplemented
I just removed the match, please tell me if you think an assertion is needed
saulecabrera
left a comment
There was a problem hiding this comment.
One minor nit and this should be good to go.
| self.asm.addv(tmp, tmp, VectorSize::Size8x8); | ||
| self.asm.mov_from_vec(tmp, src.into(), 0, OperandSize::S8); | ||
| context.stack.push(src.into()); | ||
| context.free_reg(tmp); |
There was a problem hiding this comment.
The scratch register is manually tracked i.e., free_reg is a no-op in this case, so we can remove this call.
d896ad5 to
f2e2425
Compare
Hey 👋
This PR implements
extend,wrap,popcnt,promote&demoteinstructions for winch targeting aarch64.#8321