x64: implement vselect with variable blend instructions #2905
Conversation
github-actions
bot
added
cranelift
There was a problem hiding this comment.
I've not really reviewed many cranelift backend PRs before, so I'm pretty unfamiliar with it. That being said I'm always curious to learn, and everything looks reasonable enough here, but I left some comments inline.
Could a test be added for the wasm behavior as well?
| // Variable blend instructions expect the condition mask to be | ||
| // in XMM0. | ||
| let xmm0 = Writable::from_reg(regs::xmm0()); | ||
| ctx.emit(Inst::gen_move(xmm0, condition, ty)); |
There was a problem hiding this comment.
How does regalloc know that xmm0 is live until the xmm_rm_r instruction below?
There was a problem hiding this comment.
@cfallin can correct me if I get this wrong but every generated instruction tells regalloc how it is using each register using the RegUsageMapper trait. Well, maybe it is the RegUsageCollector. In any case, regalloc should know that a move to that virtual register (hard-coded to XMM0) is a def and that that register should be left alone until it is used.
There was a problem hiding this comment.
Indeed, the regalloc.rs semantics are such that we compute live-ranges for both real regs and virtual regs; and a real reg is reserved for the extent of its virtual ranges. As long as we properly have xmm0 as a use below the move then everything works as expected!
There was a problem hiding this comment.
Yeah it makes sense to me that it's use/def based and this is a def of xmm0, but I wasn't able to find the use of xmm0 (I'm probably missing something though?) This looks like the relevant block but it doesn't seem have anything xmm0-related there, though?
(I'd sort of expect a match on the opcode to add the use of xmm0 in some cases, like the ones used below)
There was a problem hiding this comment.
Yup, actually you're right. In talking more to @cfallin I actually came to the same conclusion. Let me fix that.
There was a problem hiding this comment.
Out of curiosity, what's the failure mode for a situation like this? The register allocator sees a "def" of xmm0 but it never sees a use. Does that mean that the live range is considered infinite? Or is the live range immediately "dead" after the def?
(this seems like a worrying thing to me and easy to overlook, so curious if we could bolster up checks one way or another to prevent this from happening again)
There was a problem hiding this comment.
Yeah, the failure mode isn't great: the real-reg liverange ends at the last mention (the def) and so the register is liable to be reused for something else, clobbering the value.
Always looking for ways to foolproof this; it's a bit tricky as the question is: what is the ground-truth we can check against? If we forget to mention a register-use in the metadata we provide to the allocator, the only other way of seeing that would be to disassemble the resulting machine code independently and check its register-mentions. We could certainly do that, perhaps as another oracle during fuzzing; but it's a big project (correlating the disassembly to the VCode especially with multiple-instruction lowerings would take some care).
There was a problem hiding this comment.
Oh nah I wouldn't want to go so all-out just yet. I would imagine something much more simple which is that if something is defined and never used, it's considered either live forever or invalid, causing a panic. In the "live forever" case we'd in theory one day ask why our code was so slow and fix this by limiting the live range, and in the latter case we'd catch the panic real fast and fix it.
I'm not sure if it's common, though, for values to be defined and never used. If that happens pretty normally then there's probably not much we can do about this other than being vigilant for now.
There was a problem hiding this comment.
Interesting thoughts in any case, thanks for the ideas here!
Keeping a real-reg liverange open forever would ensure safety here but could potentially have forward-progress implications: it's essentially a "leak" of one register, and if we reserve too many real regs in this way, then we could hit a point where we don't have enough registers to satisfy constraints and finish allocation. That's less bad than clobbering but still bad, and would be somewhat hard to debug.
In theory, dead defs shouldn't happen at the CLIF level, because we lower based on demand, though it's possible at the VCode level depending on the lowering pattern. There's also the issue that we use dead defs as a way to encode clobbers (at calls, for example), so we'd need a different category for those, but it could be doable.
We can think more about this -- I'll add the disallow-dead-defs idea to my running list of "things to look at next time I focus on fuzzing infra" :-)
| let opcode = match condition_ty { | ||
| types::B64X2 => SseOpcode::Blendvpd, | ||
| types::B32X4 => SseOpcode::Blendvps, | ||
| types::B16X8 | types::B8X16 => SseOpcode::Pblendvb, |
There was a problem hiding this comment.
I was curious after seeing two of these get mapped to the same instruction, while the above two types have different instructions. Reading the description, couldn't pblendvb be used for all 4 types instead of just these two smaller ones?
There was a problem hiding this comment.
This is purely a preference thing like I've done previously on moves, e.g.: it adds a negligible overhead to compilation but it matches the original intent of the instruction more closely, which I feel helps during debugging and sometimes can help with latency/throughput. In this case, the latency/throughput differences are very minor (see IA Optimization Manual, appendix D; depending on the arch family, 1 in some cases and 2 in others).
There was a problem hiding this comment.
Ah ok makes sense! Presumably there's no dedicated instruction for 16-bit types?
Could you leave a comment for how pblendvb would be correct for all of these but more specialized versions are used for slight optimizations?
There was a problem hiding this comment.
Yup, no 16-bit, at least in SSE-land (VPBLENDM* exists up in AVX512). Yeah, I'll add the comment.
This change implements `vselect` using SSE4.1's `BLENDVPS`, `BLENDVPD`, and `PBLENDVB`. `vselect` is a lane-selecting instruction that is used by [simple_preopt.rs](https://github.com/bytecodealliance/wasmtime/blob/fa1faf5d224b9640eb33eed97e6a890da23afa33/cranelift/codegen/src/simple_preopt.rs#L947-L999) to lower `bitselect` to a single x86 instruction when the condition mask is known to be boolean (all 1s or 0s, e.g., from a conversion). This is better than `bitselect` in general, which lowers to 4-5 instructions. The old backend had the `vselect` lowering; this simply introduces it to the new backend.
|
@alexcrichton, one last comment I forgot to address before this gets merged:
Most of the Wasm side of things is tested using the Wasm SIMD spec tests. I think this type of thing is already covered somewhere in |
This change implements
vselectusing SSE4.1'sBLENDVPS,BLENDVPD, andPBLENDVBto fix #2754.vselectis a lane-selecting instruction that is used by simple_preopt.rs to lowerbitselectto a single x86 instruction when the condition mask is known to be boolean (all 1s or 0s, e.g., from a conversion). This is better thanbitselectin general, which lowers to 4-5 instructions. The old backend had thevselectlowering; this simply introduces it to the new backend.