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: Implied branch is not removed #111725

Open
EgorBo opened this issue Jan 22, 2025 · 4 comments · May be fixed by #111766
Open

JIT: Implied branch is not removed #111725

EgorBo opened this issue Jan 22, 2025 · 4 comments · May be fixed by #111766
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@EgorBo
Copy link
Member

EgorBo commented Jan 22, 2025

Here two functions do the same thing but their codegen is different:

public static void WriteBigEndian128_1(ulong hi, ulong lo, Span<byte> destination)
{
    if (destination.Length < 16)
        return;

    Span<byte> destHi = destination.Slice(0, 8);
    Span<byte> destLo = destination.Slice(8, 8);
    BinaryPrimitives.WriteUInt64BigEndian(destLo, lo);
    BinaryPrimitives.WriteUInt64BigEndian(destHi, hi);
}

public static void WriteBigEndian128_2(ulong hi, ulong lo, Span<byte> destination)
{
    if (destination.Length >= 16)
    {
        Span<byte> destHi = destination.Slice(0, 8);
        Span<byte> destLo = destination.Slice(8, 8);
        BinaryPrimitives.WriteUInt64BigEndian(destLo, lo);
        BinaryPrimitives.WriteUInt64BigEndian(destHi, hi);
    }
}
; Method Benchmarks:WriteBigEndian128_1(ulong,ulong,System.Span`1[ubyte]) (FullOpts)
G_M15399_IG01:  
       sub      rsp, 40
G_M15399_IG02:  
       mov      rax, bword ptr [r8]
       mov      r8d, dword ptr [r8+0x08]
       cmp      r8d, 16
       jl       SHORT G_M15399_IG04
G_M15399_IG03:  
       cmp      r8d, 8
       jl       SHORT G_M15399_IG05
       lea      r8, bword ptr [rax+0x08]
       bswap    rdx
       mov      qword ptr [r8], rdx
       bswap    rcx
       mov      qword ptr [rax], rcx
G_M15399_IG04:  
       add      rsp, 40
       ret      
G_M15399_IG05:  
       call     [System.ThrowHelper:ThrowArgumentOutOfRangeException()]
       int3     
; Total bytes of code: 51


; Method Benchmarks:WriteBigEndian128_2(ulong,ulong,System.Span`1[ubyte]) (FullOpts)
G_M24932_IG01:  
G_M24932_IG02:  
       mov      rax, bword ptr [r8]
       mov      r8d, dword ptr [r8+0x08]
       cmp      r8d, 16
       jl       SHORT G_M24932_IG04
G_M24932_IG03:  
       lea      r8, bword ptr [rax+0x08]
       bswap    rdx
       mov      qword ptr [r8], rdx
       bswap    rcx
       mov      qword ptr [rax], rcx
G_M24932_IG04:  
       ret      
; Total bytes of code: 30

cc @AndyAyersMS @dotnet/jit-contrib

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 22, 2025
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jan 22, 2025
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@EgorBo EgorBo added this to the Future milestone Jan 22, 2025
@EgorBo EgorBo removed the untriaged New issue has not been triaged by the area owner label Jan 22, 2025
@EgorBo
Copy link
Member Author

EgorBo commented Jan 22, 2025

Smaller repro:

public static void Test(uint x)
{
    if (x < 16)
        return;

    if (x < 8)
        Console.WriteLine(); // should be eliminated as never reachable
}
; Method Benchmarks:Test(uint) (FullOpts)
G_M52469_IG01:  
       sub      rsp, 40
G_M52469_IG02:  
       cmp      ecx, 16
       jb       SHORT G_M52469_IG04
G_M52469_IG03:  
       cmp      ecx, 8
       jae      SHORT G_M52469_IG04
       call     [System.Console:WriteLine()]
G_M52469_IG04:  
       nop      
G_M52469_IG05:  
       add      rsp, 40
       ret      
; Total bytes of code: 26

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Jan 22, 2025

The logic in optRelopTryInferWithOneEqualOperand is missing for the "true" side for the dominating oper. Here we have x >= 16 dominating x >= 8.

    // Check whether the dominating compare being "false" implies the dominated compare is known
    // to be either "true" or "false".
    RelopResult treeOperStatus = IsCmp2ImpliedByCmp1(GenTree::ReverseRelop(domOper), domCns, treeOper, treeCns);
    if (treeOperStatus == RelopResult::Unknown)
    {
        return false;
    

@AndyAyersMS
Copy link
Member

Looks like this is not too hard to add. I had to relax an assert because now we have cases where the dominated relop is fully redundant (canInferFromTrue and canInferFromFalse are both true), eg

  if (x < 47)
  {
      if (x <= 46)

which we didn't see from other forms of predicate inference. (arguably, perhaps, VN should be equating these, eg normalizing all LTs to LEs, at least for cases involving constants where we can avoid getting into trouble with overflow).

AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Jan 23, 2025
Thanks to dotnet#95234, RBO can draw inferences when the same value is compared to
different constants, if the initial comparison dominates and was false. Generalize this
to also handle cases where the initial comparison dominates and is true.

Fixes dotnet#111725.
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Jan 23, 2025
@EgorBo EgorBo modified the milestones: Future, 10.0.0 Jan 23, 2025
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 in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants