Winch: Float conversion instructions #15
Conversation
jeffcharles
force-pushed
the
winch-float-conversion-instructions
branch
from
bc02d7d
to
1c0607c
Compare
jeffcharles
force-pushed
the
winch-float-conversion-instructions
branch
2 times, most recently
from
c33ab59
to
39c585d
Compare
|
Fuzzing has been running successfully for around 4 hours |
winch/codegen/src/masm.rs
Outdated
| /// Truncate a float into an integer. | ||
| /// In x64, this will emit multiple instructions. |
There was a problem hiding this comment.
Could we generalize these comments in such a way that are able to omit ISA-specific details? Something like:
/// Emits one or more instructions to truncate a float into an integer.
There was a problem hiding this comment.
I think it would be less error prone (and potentially less tedious) to do this than having to update the comment every time a new backend is added.
winch/codegen/src/isa/x64/masm.rs
Outdated
| fn truncate( | ||
| &mut self, | ||
| context: &mut CodeGenContext, | ||
| src_size: OperandSize, | ||
| dst_size: OperandSize, | ||
| kind: TruncateKind, | ||
| ) { |
There was a problem hiding this comment.
I'll add this comment here, but it applies to all the new methods which take a &mut CodeGenContext as a parameter.
I'm trying to understand why passing the &mut CodeGenContext is needed, versus passing the already resolved register operands. The general convention that I'm trying to keep at the MacroAssembler level is that the approach of passing the CodeGenContext should be used as a last resort, more concretely when the lowering of a particular instruction is not generic enough, and needs resolution of ISA-specific details, like for example in the case of the div instruction, which in x64 requires very specific registers to be used. Looking at the conversion and truncation methods, it doesn't look like there's anything ISA specific here and that most of the logic can be moved to the visitor, which would be my preference. Another advantage of such approach is that by keeping the MacroAssembler layer as thin as possible, there's less repetition that will be needed when working on other backends.
There was a problem hiding this comment.
Let me know if there's anything that I'm missing here.
There was a problem hiding this comment.
Yeah I can move the register part of the logic into the visitor. I wasn't entirely sure what your preference would be so I went with one approach but agreed that since it's not ISA-specific, having in the visitor makes more sense.
There was a problem hiding this comment.
Actually for truncate, I need an additional temporary XMM register but only when performing an unsigned truncation. I could change how we model truncation so we have a signed_truncate and an unsigned_truncate at the MacroAssembler level where the unsigned_truncate method would take an additional temporary XMM parameter.
There was a problem hiding this comment.
The same pattern holds for convert where I need an additional temporary GPR but only when performing an unsigned convert. Again I could split that method at the MacroAssembler level into a signed_convert and an unsigned_convert where the unsigned_convert takes an additional temporary GPR as a parameter.
There was a problem hiding this comment.
Yeah, I didn't mention it explicitly on my original comment but I think that splitting those operations (<signed|unsigned>_convert) and perform the dispatch from the visitor makes sense to me.
There was a problem hiding this comment.
Sorry for rambling on this, it's been a few weeks since I worked on this so I'm just remembering I did this as I go through the exercise again. I don't know if we would need the extra temporary XMM register or GPR for other ISAs when performing unsigned truncation or conversion. Maybe that's just an x86 detail? But I haven't tried making an AArch64 implementation yet so maybe it would also be necessary on that ISA. So does it make sense to bake in needing additional temporary registers for unsigned truncation and conversion at the MacroAssembler trait level given it may just be an x86 requirement?
There was a problem hiding this comment.
I think it makes sense to do that. When implementing the aarch64 backend we'll know for sure and we can adjust then. If an extra register is the issue, there are other options to deal with that IMO, like for example passing an smallvec of registers to work with, in which each backend defines how many are needed, which could be empty if no extra registers are needed.
|
Fuzzing has been running for a little over two hours successfully |
winch/codegen/src/codegen/context.rs
Outdated
| /// Prepares arguments for emitting a convert operation with a temporary | ||
| /// GPR. | ||
| pub fn convert_op_with_tmp_gpr<F, M>(&mut self, masm: &mut M, dst_ty: WasmType, mut emit: F) | ||
| where | ||
| F: FnMut(&mut M, Reg, Reg, Reg, OperandSize), | ||
| M: MacroAssembler, | ||
| { | ||
| let tmp_gpr = self.any_gpr(masm); | ||
| self.convert_op(masm, dst_ty, |masm, dst, src, dst_size| { | ||
| emit(masm, dst, src, tmp_gpr, dst_size); | ||
| }); | ||
| self.free_reg(tmp_gpr); | ||
| } | ||
|
|
||
| /// Prepares arguments for emitting a convert operation with a temporary | ||
| /// floating point register. | ||
| pub fn convert_op_with_tmp_fpr<F, M>(&mut self, masm: &mut M, dst_ty: WasmType, mut emit: F) | ||
| where | ||
| F: FnMut(&mut M, Reg, Reg, Reg, OperandSize), | ||
| M: MacroAssembler, | ||
| { | ||
| let tmp_fpr = self.reg_for_class(RegClass::Float, masm); | ||
| self.convert_op(masm, dst_ty, |masm, dst, src, dst_size| { | ||
| emit(masm, dst, src, tmp_fpr, dst_size); | ||
| }); | ||
| self.free_reg(tmp_fpr); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think it's possible to merge these two functions into one that takes an extra RegClass param to signal the class of the temporary register?
There was a problem hiding this comment.
Yes. I didn't do that initially since I was thinking it wouldn't make sense to expose that to the visitor but I'm fine with making that change.
|
I'll squash the commits and open a PR on the main repo |
jeffcharles
force-pushed
the
winch-float-conversion-instructions
branch
from
d441b02
to
5d783c6
Compare
No description provided.