From 34b72a351745aa0d47bb0b74ebcd0f0a616d613d Mon Sep 17 00:00:00 2001 From: "Field G. Van Zee" Date: Fri, 23 Feb 2018 16:33:32 -0600 Subject: [PATCH] Fixed obscure read-beyond-bounds bug in sgemm ukrs. Details: - Fixed an obscure bug in the bli_sgemm_haswell_asm_6x16 and bli_sgemm_zen_asm_6x16 microkernels when the input/output matrix C is stored with general stride (ie: both rs and cs are non-unit). The bug was rooted in the way those microkernels read from matrix C-- namely, they used vmovlps/vmovhps instead of movss. By loading two floats at a time, even if one of them was treated as junk, the assembly code could be written in a more concise manner. However, under certain conditions--if m % mr == 0 and n % nr == 0 and the underlying matrix is not an internal "view" into a larger matrix-- this could result in the very last vmovhps of the last (bottom-right) microkernel invocation reading beyond valid memory. Specifically, the low 32 bits read would always be valid, but the high 32 bits could reside beyond the bounds of the array in which the output C matrix is contained. To remedy this situation, we now selectively use movss to load any element that could be the last element in the matrix. --- kernels/haswell/3/bli_gemm_haswell_asm_d6x8.c | 11 ++++++++++- kernels/zen/3/bli_gemm_zen_asm_d6x8.c | 11 ++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/kernels/haswell/3/bli_gemm_haswell_asm_d6x8.c b/kernels/haswell/3/bli_gemm_haswell_asm_d6x8.c index a1616ec7d..3f8e4ea4a 100644 --- a/kernels/haswell/3/bli_gemm_haswell_asm_d6x8.c +++ b/kernels/haswell/3/bli_gemm_haswell_asm_d6x8.c @@ -43,8 +43,17 @@ "vshufps $0x88, %%xmm1, %%xmm0, %%xmm0 \n\t" \ "vmovlps (%%rcx,%%rsi,4), %%xmm2, %%xmm2 \n\t" \ "vmovhps (%%rcx,%%r15 ), %%xmm2, %%xmm2 \n\t" \ + /* We can't use vmovhps for loading the last element becauase that + might result in reading beyond valid memory. (vmov[lh]psd load + pairs of adjacent floats at a time.) So we need to use vmovss + instead. But since we're limited to using ymm0 through ymm2 + (ymm3 contains beta and ymm4 through ymm15 contain the microtile) + and due to the way vmovss zeros out all bits above 31, we have to + load element 7 before element 6. */ \ + "vmovss (%%rcx,%%r10 ), %%xmm1 \n\t" \ + "vpermilps $0xcf, %%xmm1, %%xmm1 \n\t" \ "vmovlps (%%rcx,%%r13,2), %%xmm1, %%xmm1 \n\t" \ - "vmovhps (%%rcx,%%r10 ), %%xmm1, %%xmm1 \n\t" \ + /*"vmovhps (%%rcx,%%r10 ), %%xmm1, %%xmm1 \n\t"*/ \ "vshufps $0x88, %%xmm1, %%xmm2, %%xmm2 \n\t" \ "vperm2f128 $0x20, %%ymm2, %%ymm0, %%ymm0 \n\t" diff --git a/kernels/zen/3/bli_gemm_zen_asm_d6x8.c b/kernels/zen/3/bli_gemm_zen_asm_d6x8.c index 774f33fea..accc70af1 100644 --- a/kernels/zen/3/bli_gemm_zen_asm_d6x8.c +++ b/kernels/zen/3/bli_gemm_zen_asm_d6x8.c @@ -44,8 +44,17 @@ "vshufps $0x88, %%xmm1, %%xmm0, %%xmm0 \n\t" \ "vmovlps (%%rcx,%%rsi,4), %%xmm2, %%xmm2 \n\t" \ "vmovhps (%%rcx,%%r15 ), %%xmm2, %%xmm2 \n\t" \ + /* We can't use vmovhps for loading the last element becauase that + might result in reading beyond valid memory. (vmov[lh]psd load + pairs of adjacent floats at a time.) So we need to use vmovss + instead. But since we're limited to using ymm0 through ymm2 + (ymm3 contains beta and ymm4 through ymm15 contain the microtile) + and due to the way vmovss zeros out all bits above 31, we have to + load element 7 before element 6. */ \ + "vmovss (%%rcx,%%r10 ), %%xmm1 \n\t" \ + "vpermilps $0xcf, %%xmm1, %%xmm1 \n\t" \ "vmovlps (%%rcx,%%r13,2), %%xmm1, %%xmm1 \n\t" \ - "vmovhps (%%rcx,%%r10 ), %%xmm1, %%xmm1 \n\t" \ + /*"vmovhps (%%rcx,%%r10 ), %%xmm1, %%xmm1 \n\t"*/ \ "vshufps $0x88, %%xmm1, %%xmm2, %%xmm2 \n\t" \ "vperm2f128 $0x20, %%ymm2, %%ymm0, %%ymm0 \n\t"