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

Jit doesn't always eliminate identical checks #11909

Closed
benaadams opened this issue Jan 28, 2019 · 3 comments · Fixed by #43811
Closed

Jit doesn't always eliminate identical checks #11909

benaadams opened this issue Jan 28, 2019 · 3 comments · Fixed by #43811
Assignees
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 optimization
Milestone

Comments

@benaadams
Copy link
Member

Double check is via outer method (first) and then inlined method (second, Span .ctor)

Seen in dotnet/coreclr#22207 (comment)

; Assembly listing for method String:EndsWith(ref,int):bool:this
...
G_M23680_IG10:
       mov      ecx, dword ptr [rsi+8]
       sub      ecx, r8d
       cmp      dword ptr [rsi+8], ecx   ; should eliminate one below?
       jb       SHORT G_M23680_IG15
       cmp      dword ptr [rsi+8], ecx   ; should be eliminated?
       jb       G_M23680_IG22
G_M23680_IG11:

/cc @AndyAyersMS

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

@benaadams
Copy link
Member Author

From dotnet/coreclr#22297

class Program
{
    static void Main() { Positive1(42); Positive2(42); }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static bool Positive1(int? i) => i.HasValue && i.Value > 0;
}
G_M60389_IG02:
       movzx    rax, byte  ptr [rsp+30H]
       test     eax, eax
       je       SHORT G_M60389_IG05   ; should eliminate one below?
       test     eax, eax
       je       SHORT G_M60389_IG07   ; should be eliminated?

@benaadams benaadams changed the title Jit doesn't always eliminate identical checks (bounds) Jit doesn't always eliminate identical checks Jan 30, 2019
@AndyAyersMS
Copy link
Member

Probably not going to get fixed in 3.0, but let me take a look first.

@AndyAyersMS
Copy link
Member

ForString:EndsWith(ref,int):bool:this -- the jit value numbers the two compares the same, but it looks like assertion prop won't touch this as the value being compared to the bound comes from a subtract.

Wondering how well a simple demand driven walk back up the dominator tree would do for things like this.... if a dom pred ends in a JTRUE and its compare node value numbers the same, we should be able to deduce the outcome of the dominated compare.

Then we wouldn't be so reliant on assertion prop trying to guess which facts were interesting.

Not likely to get to this in 3.0, but might not be all that hard.

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Oct 25, 2020
For a relop, walk up the dominator tree to see if any dominating
block has a similar compare. If so, and there's just one path from
that block to the relop, the relop's value is known.

Closes dotnet#11909.
@AndyAyersMS AndyAyersMS modified the milestones: Future, 6.0.0 Oct 26, 2020
@AndyAyersMS AndyAyersMS self-assigned this Oct 26, 2020
@BruceForstall BruceForstall added the JitUntriaged CLR JIT issues needing additional triage label Oct 28, 2020
@JulieLeeMSFT JulieLeeMSFT removed the JitUntriaged CLR JIT issues needing additional triage label Nov 2, 2020
AndyAyersMS added a commit that referenced this issue Nov 4, 2020
For a relop, walk up the dominator tree to see if any dominating
block has a similar compare. If so, and there's just one path from
that block to the relop, the relop's value is known.

Closes #11909.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 optimization
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants