Differential D51542
[X86] Remove wrong ReadAdvance from multiclass sse_fp_unop_s Authored by andreadb on Aug 31 2018, 7:56 AM.
Details A ReadAdvance was incorrectly added to the list of SchedReadWrite for the following opcodes: sqrtss sqrtsd rsqrtss rcpss As a consequence, a wrong operand latency was computed for the register operand used by the folded load. This patch removes the wrong ReadAdvance, and updates the llvm-mca test cases. Now the llvm-mca timeline report shows correct timings for those unary fp SSE1/SSE2 instructions. Please let me know if okay to commit. Thanks,
Diff Detail
Event TimelineComment Actions I think this requires an understanding of the intent of ReadAfterLd: // Instructions with folded loads need to read the memory operand immediately, // but other register operands don't have to be read until the load is ready. // These operands are marked with ReadAfterLd. ...that D51534 did not. That's because a broadcast only has one source operand, so ReadAfterLd doesn't even make sense on that instruction? In this case, we have 2 source operands:
The patch has the intended effect of making the math op depend on the load operand, but it's not clear to me what is or should be happening in a case like this on skylake: Trunk: [0,0] DeeeeeeeeeER. dppd $1, %xmm1, %xmm2 <--- long latency, but pipelined [0,1] D=eE-------R. leaq 8(%rsp,%rdi,2), %rax [0,2] D=eeeeeeeeeER rsqrtss (%rax), %xmm2 <--- wrong: this can't start executing before %rax is loaded Apply this patch (remove ReadAfterLd:) [0,0] DeeeeeeeeeER . dppd $1, %xmm1, %xmm2 [0,1] D=eE-------R . leaq 8(%rsp,%rdi,2), %rax [0,2] D==eeeeeeeeeER rsqrtss (%rax), %xmm2 <--- is this right? the calc can begin before xmm2 is known? But with AVX the 2nd source is explicit, and ReadAfterLd has a different effect: [0,0] DeeeeeeeeeER . vdppd $1, %xmm0, %xmm1, %xmm2 [0,1] D=eE-------R . leaq 8(%rsp,%rdi,2), %rax [0,2] D====eeeeeeeeeER vrsqrtss (%rax), %xmm2, %xmm3 <--- execution delayed by vdppd? No ReadAfterLd: [0,0] DeeeeeeeeeER . . vdppd $1, %xmm0, %xmm1, %xmm2 [0,1] D=eE-------R . . leaq 8(%rsp,%rdi,2), %rax [0,2] D=========eeeeeeeeeER vrsqrtss (%rax), %xmm2, %xmm3 <--- execution delayed until xmm2 is known Comment Actions Hi Sanjay, Thanks for the feedback. Not only it didn't make any sense. It was even harmful because it was decreasing the use latency of the register used as the base address for the folded load by 'ReadAfterLd' cycles..
I think you are getting confused. These are not instructions with 2 input operands. def m : I<opc, MRMSrcMem, (outs RC:$dst), (ins x86memop:$src1)
Here rsqrtss cannot start if %rax is not available.
Here the use latency of %xmm2 is decreased by a number of cycles specified by the read-advance. def : ReadAdvance<ReadAfterLd, 5>; So the vrsqrtss can assume that %xmm2 is available 5 cycles in advance (i.e the use latency is decreased by 5 cycles).
My patch doesn't modify/remove that ReadAfterLd. It would be wrong. I hope this helps Comment Actions Sorry. You are right. The output register is only partially updated. That being said, the ReadAfterLd is still wrong as it applies to the register containing the base address of $src1. Comment Actions There seems to be 2 things here - (1) ReadAfterLd is being used incorrectly to try to make a dependency on the dst reg and (2) we have no mechanism to handle partial dependencies like for the scalar sse instructions. AFAICT this patch fixes the (1) issue but (2) isn't handled at all by the scheduler models yet.
Comment Actions Yep. I will raise a bug for 2. This patch only addresses the issue with ReadAfterLd being incorrectly applied to a register used as the base address of the folded load.
Comment Actions Thanks! I think everyone agrees that the ReadAfterLd is not behaving as intended on the SSE instructions. I am still wondering about the AVX case (and the SSE intrinsic is the same?): With ReadAfterLd: [0,0] DeeeeeeeeeER . vdppd $1, %xmm0, %xmm1, %xmm2 [0,1] D=eE-------R . leaq 8(%rsp,%rdi,2), %rax [0,2] D====eeeeeeeeeER vrsqrtss (%rax), %xmm2, %xmm3 <--- execution delayed by vdppd? No ReadAfterLd: [0,0] DeeeeeeeeeER . . vdppd $1, %xmm0, %xmm1, %xmm2 [0,1] D=eE-------R . . leaq 8(%rsp,%rdi,2), %rax [0,2] D=========eeeeeeeeeER vrsqrtss (%rax), %xmm2, %xmm3 <--- execution delayed until xmm2 is known Do we have confirmation from hardware testing or Intel docs that the 1st timeline is correct? We can punt this question to another patch to not delay this one if needed, but I'd like to understand what the hardware does in this case. Comment Actions VRSQRTSS depends on XMM2, so it delayed by vdppd. I don't have a skylake to microbenchmark that sequence. However, (in my mental model) it kind of makes sense to have a ReadAfterLd there. I would be a "strange" otherwise. Comment Actions Note: we can observe similar behavior with btver2 as the CPU model with llvm-mca (because the ReadAfterLd is on the default instruction definition): [0,0] DeeeeeeeeeER . vdppd $1, %xmm0, %xmm1, %xmm2 [0,1] .DeeE------R . leaq 8(%rsp,%rdi,2), %rax [0,2] . D====eeeeeeeER vrsqrtss (%rax), %xmm2, %xmm3 And I was thinking the opposite - it seems strange to me that hardware would devote resources to make this case faster, but maybe it's not as difficult as I'm imagining. We don't need to hold this patch up before we answer that, so LGTM. | ||||||||||||