Needless clearing of top 32 bits after vmovmaskps

[damageboy]

Repro Repo:

https://github.com/damageboy/coreclr-jit-why-mov-now

Relevant piece of code:

https://github.com/damageboy/coreclr-jit-why-mov-now/blob/09e990e4d87190e33ca9ea1a794ec045a6651f97/Program.cs#L37-L44

            maskyMcMaskFace  = (ulong) (uint) MoveMask(CompareGreaterThan(L0, P).AsSingle()) << 00;
            maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L1, P).AsSingle()) << 08;
            maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L2, P).AsSingle()) << 16;
            maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L3, P).AsSingle()) << 24;
            maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L4, P).AsSingle()) << 32;
            maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L5, P).AsSingle()) << 40;
            maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L6, P).AsSingle()) << 48;
            maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L7, P).AsSingle()) << 56;

Generated asm:

https://github.com/damageboy/coreclr-jit-why-mov-now/blob/09e990e4d87190e33ca9ea1a794ec045a6651f97/listing.asm#L57-L115

;             maskyMcMaskFace  = (ulong) (uint) MoveMask(CompareGreaterThan(L0, P).AsSingle()) << 00;
00007F05AE6B097A C5F566C8             vpcmpgtd ymm1,ymm1,ymm0
00007F05AE6B097E C5FC50F9             vmovmskps edi,ymm1
00007F05AE6B0982 8BFF                 mov     edi,edi


;             maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L1, P).AsSingle()) << 08;
00007F05AE6B0984 C5ED66C8             vpcmpgtd ymm1,ymm2,ymm0
00007F05AE6B0988 C5FC50C1             vmovmskps eax,ymm1
00007F05AE6B098C 8BC0                 mov     eax,eax
00007F05AE6B098E 48C1E008             shl     rax,8
00007F05AE6B0992 480BF8               or      rdi,rax


;             maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L2, P).AsSingle()) << 16;
00007F05AE6B0995 C5E566C8             vpcmpgtd ymm1,ymm3,ymm0
00007F05AE6B0999 C5FC50C1             vmovmskps eax,ymm1
00007F05AE6B099D 8BC0                 mov     eax,eax
00007F05AE6B099F 48C1E010             shl     rax,10h
00007F05AE6B09A3 480BC7               or      rax,rdi
00007F05AE6B09A6 488BF8               mov     rdi,rax


;             maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L3, P).AsSingle()) << 24;
00007F05AE6B09A9 C5DD66C8             vpcmpgtd ymm1,ymm4,ymm0
00007F05AE6B09AD C5FC50C1             vmovmskps eax,ymm1
00007F05AE6B09B1 8BC0                 mov     eax,eax
00007F05AE6B09B3 48C1E018             shl     rax,18h
00007F05AE6B09B7 480BC7               or      rax,rdi
00007F05AE6B09BA 488BF8               mov     rdi,rax


;             maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L4, P).AsSingle()) << 32;
00007F05AE6B09BD C5D566C8             vpcmpgtd ymm1,ymm5,ymm0
00007F05AE6B09C1 C5FC50C1             vmovmskps eax,ymm1
00007F05AE6B09C5 8BC0                 mov     eax,eax
00007F05AE6B09C7 48C1E020             shl     rax,20h
00007F05AE6B09CB 480BC7               or      rax,rdi
00007F05AE6B09CE 488BF8               mov     rdi,rax


;             maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L5, P).AsSingle()) << 40;
00007F05AE6B09D1 C5CD66C8             vpcmpgtd ymm1,ymm6,ymm0
00007F05AE6B09D5 C5FC50C1             vmovmskps eax,ymm1
00007F05AE6B09D9 8BC0                 mov     eax,eax
00007F05AE6B09DB 48C1E028             shl     rax,28h
00007F05AE6B09DF 480BC7               or      rax,rdi
00007F05AE6B09E2 488BF8               mov     rdi,rax


;             maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L6, P).AsSingle()) << 48;
00007F05AE6B09E5 C5C566C8             vpcmpgtd ymm1,ymm7,ymm0
00007F05AE6B09E9 C5FC50C1             vmovmskps eax,ymm1
00007F05AE6B09ED 8BC0                 mov     eax,eax
00007F05AE6B09EF 48C1E030             shl     rax,30h
00007F05AE6B09F3 480BC7               or      rax,rdi
00007F05AE6B09F6 488BF8               mov     rdi,rax


;             maskyMcMaskFace |= (ulong) (uint) MoveMask(CompareGreaterThan(L7, P).AsSingle()) << 56;
00007F05AE6B09F9 C5BD66C0             vpcmpgtd ymm0,ymm8,ymm0
00007F05AE6B09FD C5FC50C0             vmovmskps eax,ymm0
00007F05AE6B0A01 8BC0                 mov     eax,eax
00007F05AE6B0A03 48C1E038             shl     rax,38h
00007F05AE6B0A07 480BC7               or      rax,rdi
00007F05AE6B0A0A 488BF8               mov     rdi,rax

Issue

In the asm listing, There is a mov eax,eax between the vmovmskps
and the shl opcodes.

Unless I'm gravely mistaken, this is superfluous?

The Intel docs are a bit fuzzy on this, I admit:
VMOVMSKPS (VEX.256 encoded version)

DEST[0] 🡐 SRC[31]
DEST[1] 🡐 SRC[63]
DEST[2] 🡐 SRC[95]
DEST[3] 🡐 SRC[127]
DEST[4] 🡐 SRC[159]
DEST[5] 🡐 SRC[191]
DEST[6] 🡐 SRC[223]
DEST[7] 🡐 SRC[255]
IF DEST = r32
    THEN DEST[31:8] 🡐 0;
    ELSE DEST[63:8] 🡐 0;
FI

But it would seem that the entire register is cleared in 64 bit mode.
My reasoning for this, is that I didn't manage to generate both a 32 and a 64 bit variant of
vmovmaskps {e,r}ax,ymm0

That is to say, whenever I try to encode both:

vmovmskps rax,ymm0
vmovmskps eax,ymm0

I always get the same instruction stream:

$ cat x.asm
vmovmskps rax,ymm0
vmovmskps eax,ymm0

$ nasm -f elf64 x.asm

$ objdump -D -Mintel  x.o
x.o:     file format elf64-x86-64
Disassembly of section .text:
0000000000000000 <.text>:
   0:   c5 fc 50 c0             vmovmskps eax,ymm0
   4:   c5 fc 50 c0             vmovmskps eax,ymm0

This is leading me to an understanding that there is no distinction between using a 32-bit and 64-bit register in this case... so the entire top 56 should be already cleared and the mov eax,eax is not required?

category:cq
theme:vector-codegen
skill-level:intermediate
cost:medium
impact:small

[tannergooding]

The manual calls out The default operand size is 64-bit in 64-bit mode., so this is both a case where the disassembly could be improved and where we could better track that the upper bits are already zeroed.

[damageboy]

@tannergooding Right, that explains it I guess, I missed that statement...

[mikedn]

It should be pretty easy to change the JIT to drop the cast and retype the mask node as TYP_LONG. The JIT doesn't do intrinsic optimizations currently but this can be considered a special case, the result of the intrinsic method having int return type when perhaps nuint would have made more sense.

[damageboy]

@mikedn, @tannergooding

I wouldn't mind taking a stab at this, although to say that I have a learning curve when it comes to JIT internals is an understatement.

If you could point me to the correct phase in the JIT where this should be handled, I would gladly give it a try

[mikedn]

If you could point me to the correct phase in the JIT where this should be handled, I would gladly give it a try

I think the right place would be morph (fgMorphCast likely). I have some reservations about adding stuff to morph because it's a rather hairy piece of code that needs a serious shake up but this seems trivial enough to pile on top of the existing code. It can also be done in lowering but I don't think delaying the cast removal until lowering makes sense.

[damageboy]

I've just encountered another manifestation of this issue in what I initially thought was a new code-gen issue.

In this case, the JIT not understanding that vmovmsk is indeed clearing the top 32-bits is
causing it to generate very inefficient code around vpmovzxbd/ConvertToVector256Int32() for example:

https://github.com/damageboy/coreclr-vpmovzxbd-inefficient-encoding/blob/41514073d60b064cc45cd3370b00bc33996effa7/listing.asm#L122-L135

00007FD8B99304B3 41C1E503             shl     r13d,3
00007FD8B99304B7 418BFD               mov     edi,r13d
00007FD8B99304BA C4C27D31043E         vpmovzxbd ymm0,[r14+rdi]
00007FD8B99304C0 C1E003               shl     eax,3
00007FD8B99304C3 C4C27D310C06         vpmovzxbd ymm1,[r14+rax]
00007FD8B99304C9 C5FDFEC1             vpaddd  ymm0,ymm0,ymm1
00007FD8B99304CD 41C1E403             shl     r12d,3
00007FD8B99304D1 418BC4               mov     eax,r12d
00007FD8B99304D4 C4C27D310C06         vpmovzxbd ymm1,[r14+rax]
00007FD8B99304DA C5F5FEC0             vpaddd  ymm0,ymm1,ymm0
00007FD8B99304DE 41C1E703             shl     r15d,3
00007FD8B99304E2 418BC7               mov     eax,r15d
00007FD8B99304E5 C4C27D310C06         vpmovzxbd ymm1,[r14+rax]
00007FD8B99304EB C5F5FEC0             vpaddd  ymm0,ymm1,ymm0

In this case, the JIT generates the redundant:

shl rXX,3
mov eax,rXX
vpmovzxbd ymm1,[r14+rax]

Instead of simply encoding:

vpmovzxbd ymm0,[r14+r13*8]
vpmovzxbd ymm1,[r14+rax*8]
vpaddd  ymm0,ymm0,ymm1
vpmovzxbd ymm1,[r14+r12*8]
vpaddd  ymm0,ymm1,ymm0
vpmovzxbd ymm1,[r14+r15*8]
vpaddd  ymm0,ymm1,ymm0

In my sorting code, due to unrolling, this accounts for about a 150 byte increase in the function size
which is about 5% of the code size. (150/2736 bytes).

I think this sort of code patter (vmovmsk + using it as an index to another table) is a rather common code path. This might mean that this issue has somehwhat greater effect than what (at least I) thought that it did before.