Suboptimal codegen when comparing sbyte and 16-bit values

[GrabYourPitchforks]

I have the following two repro methods along with their x64 codegen using the latest RyuJIT.

[MethodImpl(MethodImplOptions.NoInlining)]
public static bool MethodFoo(byte x)
{
    return ((byte)x < 128);
}

ConsoleApp12!ConsoleApp12.Program.MethodFoo(Byte):
00007ffe`2cb01630 80f980          cmp     cl,80h
00007ffe`2cb01633 0f92c0          setb    al
00007ffe`2cb01636 0fb6c0          movzx   eax,al
00007ffe`2cb01639 c3              ret

[MethodImpl(MethodImplOptions.NoInlining)]
public static bool MethodBar(byte x)
{
    return ((sbyte)x < -64);
}

ConsoleApp12!ConsoleApp12.Program.MethodBar(Byte):
00007ffe`2cb01650 0fb6c1          movzx   eax,cl
00007ffe`2cb01653 480fbec0        movsx   rax,al
00007ffe`2cb01657 83f8c0          cmp     eax,0FFFFFFC0h
00007ffe`2cb0165a 0f9cc0          setl    al
00007ffe`2cb0165d 0fb6c0          movzx   eax,al
00007ffe`2cb01660 c3              ret

The Foo method checks to see if the high bit is clear, and the Bar method checks to see if the byte has input pattern 10xxxxxx. These are both common operations when performing UTF-8 text processing. In the examples above, the Bar method has suboptimal codegen. I would instead expect the codegen to be the following.

cmp     cl,C0h
setl    al
movzx   eax,al
ret

category:cq
theme:basic-cq
skill-level:intermediate
cost:small

[mikedn]

Taking a look, the JIT surely has a problem with useless casts...

[AndyAyersMS]

@mikedn don't know if you looked yet -- from the dump it looks like morph is the culprit:

fgMorphTree BB01, stmt 1 (before)
               [000005] ------------              *  RETURN    int   
               [000003] ------------              |  /--*  CNS_INT   int    -64
               [000004] ------------              \--*  LT        int   
               [000002] ------------                 \--*  CAST      int <- byte <- int
               [000001] ------------                    \--*  LCL_VAR   ubyte  V00 arg0         

fgMorphTree BB01, stmt 1 (after)
               [000005] -----+------              *  RETURN    int   
               [000003] -----+------              |  /--*  CNS_INT   int    -64
               [000004] -----+------              \--*  LT        int   
               [000002] -----+------                 \--*  CAST      int <- byte <- int
               [000007] -----+------                    \--*  CAST      int <- ubyte <- int
               [000001] -----+------                       \--*  LCL_VAR   int    V00 arg0         

I tried following the rather tortuous logic in fgMorphCast to see where morph be going off the rails, but got lost in there and haven't found my way out.

[mikedn]

Yes, there's morph code that attempts to get rid of some cast via optNarrowTree. I've yet to understand how exactly optNarrowTree is supposed to work though, parameter names like srct and dstt do not help... I'd guess it's mostly intended to deal with LONG -> INT narrowing and fails for small int types.

Lowering is also involved in this, the fact that we end up generating cmp cl,80h in some cases is the result of Lowering::OptimizeConstCompare. That one only handles casts to UBYTE/BOOL currently but I don't see any obstacle in extending it to support BYTE as well. But morph needs to be fixed first otherwise changing lowering has no effect.

[mikedn]

And

00007ffe`2cb01630 80f980          cmp     cl,80h
00007ffe`2cb01633 0f92c0          setb    al

could be

       84C9                 test     cl, cl
       0F9CC0               setge    al

that is one byte shorter. Another one for lowering, it's kind of amazing how many such optimizations are possible around compares. OptimizeConstCompare already does quite a bit but there are still various things that are missing.

[GrabYourPitchforks]

As lower priority, there's also opportunity for improvement for 16-bit comparisons, as below.

[MethodImpl(MethodImplOptions.NoInlining)]
public static bool MethodFoo(uint x)
{
    return ((ushort)x < 0x2000U);
}

ConsoleApp12!ConsoleApp12.Program.MethodFoo(UInt32):
00007ff7`bffe1620 0fb7c1          movzx   eax,cx
00007ff7`bffe1623 3d00200000      cmp     eax,2000h
00007ff7`bffe1628 0f92c0          setb    al
00007ff7`bffe162b 0fb6c0          movzx   eax,al
00007ff7`bffe162e c3              ret

(The movzx is unnecessary, as the following instruction could just read cmp ax, 2000h. Or are 16-bit comparisons inefficient compared to 32-bit comparisons?)

[mikedn]

The movzx is unnecessary, as the following instruction could just read cmp ax, 2000h. Or are 16-bit comparisons inefficient compared to 32-bit comparisons?

Yep, 16 bit instructions can be problematic - they require a special byte prefix (66h) that makes the instruction larger and can slow down instruction decoding, at least on Intel CPUs.

Though it looks like this particular example can be converted to something else:

test ecx, E000h
setz

[GrabYourPitchforks]

Another pattern I'm seeing in some of the UTF-8 processing:

Sometimes I want to know if a byte is an ASCII character but also not a C0 control code. Basically, I want to test that it's in the range [ 0x20 .. 0x7F ]. (For my purposes, I don't consider 0x7F a C0 control character.)

The naive way to do this is with a standard single-branch range check:

if ((uint)(value - 0x20) <= (uint)(0x7F - 0x20)) { /* ... */ }

lea ebx, [eax - 20h]
cmp ebx, 5Fh
jbe target

This requires two registers instead of one and spends unnecessary time in the decoder. An idealized version would be as follows, which consumes only one register and spends minimal time in the decoder.

if ((sbyte)value >= (sbyte)0x20) { /* ... */ }

cmp al, 20h
jge target