x64: Sink constant loads into xmm instructions#5880
x64: Sink constant loads into xmm instructions#5880alexcrichton merged 1 commit intobytecodealliance:mainfrom
Conversation
A number of places in the x64 backend make use of 128-bit constants for various wasm SIMD-related instructions although most of them currently use the `x64_xmm_load_const` helper to load the constant into a register. Almost all xmm instructions, however, enable using a memory operand which means that these loads can be folded into instructions to help reduce register pressure. Automatic conversions were added for a `VCodeConstant` into an `XmmMem` value and then explicit loads were all removed in favor of forwarding the `XmmMem` value directly to the underlying instruction. Note that some instances of `x64_xmm_load_const` remain since they're used in contexts where load sinking won't work (e.g. they're the first operand, not the second for non-commutative instructions).
github-actions
bot
added
cranelift
jameysharp
left a comment
jameysharp
left a comment
There was a problem hiding this comment.
I've read through both the code and test changes here and it all looks correct to me. But I'd like @elliottt to take a look too because I'm not super confident about spotting issues with load-sinking.
It's worth noting that we don't yet have SIMD support in the Cranelift fuzzer, so we can't rely on that to catch issues with this PR.
|
True! Wasm-based fuzzing, however, should extensively test the simd proposal with differential execution against v8. |
|
To the fuzzing point, one request (if you haven't already planned it this way): could we hold off merging this until Mar 5, when the next release branch is cut, so we maximize fuzz time at 6 weeks rather than 2? I think it's great we're getting this but given history with SIMD loads I too am a bit apprehensive :-) |
|
I don't mind, but I'd also push back a bit in that #5849, #5841, and #5819 were sort of all much larger/riskier. I had good success uncovering semantic issues through fuzzing in not much time as well. Would you prefer, though, to back out those PRs and re-land after the cutoff? |
|
Ah, yeah, that's a fair point that my request isn't really consistent, and I hadn't connected the dots; given those are already in, I retract my request as I don't think this is any more risk in comparison. |
elliottt
left a comment
elliottt
left a comment
There was a problem hiding this comment.
This seems good to me!
The problems we had last time with aligned loads weren't related to constants, but arbitrary loads that we weren't guaranteed would be aligned. Given that and the requirements we put on function alignment, I'm pretty confident that this won't have the same problems that my changes last summer did :)
This catches a case that wasn't handled previously by bytecodealliance#5880 to allow a constant load to be folded into an instruction rather than forcing it to be loaded into a temporary register.
This catches a case that wasn't handled previously by #5880 to allow a constant load to be folded into an instruction rather than forcing it to be loaded into a temporary register.
4 participants
A number of places in the x64 backend make use of 128-bit constants for various wasm SIMD-related instructions although most of them currently use the
x64_xmm_load_consthelper to load the constant into a register. Almost all xmm instructions, however, enable using a memory operand which means that these loads can be folded into instructions to help reduce register pressure. Automatic conversions were added for aVCodeConstantinto anXmmMemvalue and then explicit loads were all removed in favor of forwarding theXmmMemvalue directly to the underlying instruction. Note that some instances ofx64_xmm_load_constremain since they're used in contexts where load sinking won't work (e.g. they're the first operand, not the second for non-commutative instructions).