Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Different codegen for return cond; vs if (cond) return true; return false; #8363

Open
stephentoub opened this issue Jun 15, 2017 · 4 comments
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI enhancement Product code improvement that does NOT require public API changes/additions JitUntriaged CLR JIT issues needing additional triage optimization tenet-performance Performance related issue
Milestone

Comments

@stephentoub
Copy link
Member

There's a function in corelib used for determining whether a char is whitespace:
https://source.dot.net/#System.Private.CoreLib/shared/System/Char.cs,264

        private static bool IsWhiteSpaceLatin1(char c)
        {
            if ((c == ' ') || (c >= '\x0009' && c <= '\x000d') || c == '\x00a0' || c == '\x0085')
            {
                return (true);
            }
            return (false);
        }

The JIT is generating this code for it:

G_M8395_IG01:

G_M8395_IG02:
       0FB7C1               movzx    rax, cx
       83F820               cmp      eax, 32
       7418                 je       SHORT G_M8395_IG04
       83F809               cmp      eax, 9
       7C05                 jl       SHORT G_M8395_IG03
       83F80D               cmp      eax, 13
       7E0E                 jle      SHORT G_M8395_IG04

G_M8395_IG03:
       3DA0000000           cmp      eax, 160
       7407                 je       SHORT G_M8395_IG04
       3D85000000           cmp      eax, 133
       7506                 jne      SHORT G_M8395_IG06

G_M8395_IG04:
       B801000000           mov      eax, 1

G_M8395_IG05:
       C3                   ret

G_M8395_IG06:
       33C0                 xor      eax, eax

G_M8395_IG07:
       C3                   ret

When I change it to instead be:

        private static bool IsWhiteSpaceLatin1(char c)
        {
            return ((c == ' ') || (c >= '\x0009' && c <= '\x000d') || c == '\x00a0' || c == '\x0085');
        }

the JIT instead generates:

G_M8395_IG01:

G_M8395_IG02:
       0FB7C1               movzx    rax, cx
       83F820               cmp      eax, 32
       741D                 je       SHORT G_M8395_IG05
       83F809               cmp      eax, 9
       7C05                 jl       SHORT G_M8395_IG03
       83F80D               cmp      eax, 13
       7E13                 jle      SHORT G_M8395_IG05

G_M8395_IG03:
       3DA0000000           cmp      eax, 160
       740C                 je       SHORT G_M8395_IG05
       3D85000000           cmp      eax, 133
       0F94C0               sete     al
       0FB6C0               movzx    rax, al

G_M8395_IG04:
       C3                   ret

G_M8395_IG05:
       B801000000           mov      eax, 1

G_M8395_IG06:
       C3                   ret

I'd have expected (maybe naively?) these to generate the same asm. Is it expected that they result in different asm? At least on my machine, the change results in measurable throughput difference for Char.IsWhiteSpace (which calls Char.IsWhiteSpaceLatin1), around a ~20% improvement when the char isn't whitespace.

category:cq
theme:basic-cq
skill-level:expert
cost:medium

@AndyAyersMS
Copy link
Member

The JIT currently has some limitations in this area, so the result it not surprising. These issues tend to show up more prominently when bool-valued methods are inlined into if (...) or similar predicate contexts and the return value is materialized rather than being inferred from tests.

It is definitely something we should improve on.

@mikedn
Copy link
Contributor

mikedn commented Jun 15, 2017

Same as #4207

P.S. Hmm, probably not. As far as I can tell this method is not inlined so the situation is rather different. This has more to do with the JIT simply not recognizing the pattern rather than having an intermediary lclvar that prevents it from doing that.

The little if-conversion experiment I have should recognize this.

P.P.S. Yep, with if-conversion enabled the same code is generated in both cases.

@mikedn
Copy link
Contributor

mikedn commented Jun 16, 2017

It's also worth pointing out that we don't handle range tests like (c >= '\x0009' && c <= '\x000d'), these can be reduced to a single branch by using a subtraction and an unsigned comparison.

@stephentoub
Copy link
Member Author

It's also worth pointing out that we don't handle range tests like (c >= '\x0009' && c <= '\x000d')

Good point. And manually making that change also causes it to then be inlined. I'll submit a PR to address that, too.

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@BruceForstall BruceForstall added the JitUntriaged CLR JIT issues needing additional triage label Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI enhancement Product code improvement that does NOT require public API changes/additions JitUntriaged CLR JIT issues needing additional triage optimization tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

5 participants