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

Devirtualization isn't acting on devirtualizable calls past the first #39519

Closed
NinoFloris opened this issue Jul 17, 2020 · 4 comments
Closed
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@NinoFloris
Copy link
Contributor

NinoFloris commented Jul 17, 2020

Ran from a VM due to dotnet/BenchmarkDotNet#1499

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.17763.1039 (1809/October2018Update/Redstone5)
Intel Core i7-4980HQ CPU 2.80GHz (Haswell), 1 CPU, 4 logical and 4 physical cores
.NET Core SDK=5.0.100-preview.6.20318.15
  [Host]   : .NET Core 5.0.0 (CoreCLR 5.0.20.30506, CoreFX 5.0.20.30506), X64 RyuJIT
  ShortRun : .NET Core 5.0.0 (CoreCLR 5.0.20.30506, CoreFX 5.0.20.30506), X64 RyuJIT

Job=ShortRun  IterationCount=3  LaunchCount=1  
WarmupCount=3  
Method Mean Error StdDev Code Size
Combined 3.6617 ns 0.9868 ns 0.0541 ns 42 B
Separate 3.6139 ns 0.4095 ns 0.0224 ns 42 B
JustMe 0.0000 ns 0.0000 ns 0.0000 ns 6 B

The idea is to do a two step virtual call, once dispatching from this (Combined) and once to a separate sealed class (Separate), JustMe is the control as it devirtualizes correctly.

using System;
using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using BenchmarkDotNet.Jobs;
using BenchmarkDotNet.Running;

namespace test2
{
    public interface IMe 
    {
        bool Hello();   
    }

    public interface IYou 
    {
        IMe You { get; }   
    }

    public sealed class Combined : IMe, IYou
    {
        public IMe You => this;
        public bool Hello() => true;
    }
    
    public sealed class MeImpl : IMe
    {
        public bool Hello() => true;
    }
    
    public sealed class YouImpl : IYou
    {
        public IMe You => new MeImpl();
    }

    [ShortRunJob, DisassemblyDiagnoser(exportGithubMarkdown: true)]
    public class Program
    {
        [Benchmark]
        public bool Combined() => new Combined().You.Hello();
        
        [Benchmark]
        public bool Separate() => new YouImpl().You.Hello();
        
        [Benchmark]
        public bool JustMe() => new MeImpl().Hello();
        
        static void Main(string[] args)
        {
            BenchmarkRunner.Run<Program>();
        }
    }
}

And the resulting asm:

.NET Core 5.0.0 (CoreCLR 5.0.20.30506, CoreFX 5.0.20.30506), X64 RyuJIT

; test2.Program.Combined()
       sub       rsp,28
       mov       rcx,offset MT_test2.Combined
       call      CORINFO_HELP_NEWSFAST
       mov       rcx,rax
       mov       rax,[7FFA0807B738]
       add       rsp,28
       jmp       rax
; Total bytes of code 36
; test2.Combined.Hello()
       mov       eax,1
       ret
; Total bytes of code 6

.NET Core 5.0.0 (CoreCLR 5.0.20.30506, CoreFX 5.0.20.30506), X64 RyuJIT

; test2.Program.Separate()
       sub       rsp,28
       mov       rcx,offset MT_test2.MeImpl
       call      CORINFO_HELP_NEWSFAST
       mov       rcx,rax
       mov       rax,[7FFA0808B7F0]
       add       rsp,28
       jmp       rax
; Total bytes of code 36
; test2.MeImpl.Hello()
       mov       eax,1
       ret
; Total bytes of code 6

.NET Core 5.0.0 (CoreCLR 5.0.20.30506, CoreFX 5.0.20.30506), X64 RyuJIT

; test2.Program.JustMe()
       mov       eax,1
       ret
; Total bytes of code 6

Expectation would be that all benchmarks compile down to mov eax, 1

category:cq
theme:devirtualization
skill-level:expert
cost:large
impact:medium

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Jul 17, 2020
@AndyAyersMS
Copy link
Member

It's not readily apparent from the disassembly above, but both calls are in fact devirtualized. But this happens "late" as the object being dispatched is the return value from a call, and because of this, we currently can't do inlining or box removal optimizations.

;; early attempt during importation

impDevirtualizeCall: Trying to devirtualize interface call:
    class for 'this' is IMe (attrib 00200400)
    base method is IMe::Hello
No unique implementor of interface 00000000D1FFAB1E (IMe), sorry

;; late attempt after inlining

impDevirtualizeCall: Trying to devirtualize interface call:
    class for 'this' is Combined [exact] (attrib 20000010)
    base method is IMe::Hello
--- base class is interface
    devirt to Combined::Hello -- exact
               [000009] --C-G-------              *  CALLV stub int    IMe.Hello
               [000006] ------------ this in rcx  \--*  LCL_VAR   ref    V02 tmp1         
    exact; can devirtualize
... after devirt...
               [000009] --C-G-------              *  CALL nullcheck int    Combined.Hello
               [000006] ------------ this in rcx  \--*  LCL_VAR   ref    V02 tmp1         

The calls end up getting invoked via an indirection cell which makes them look like they're possibly still virtual.

@AndyAyersMS
Copy link
Member

Linked this into the devirtualization "todo" issue #7541. Marking as future.

@AndyAyersMS AndyAyersMS removed the untriaged New issue has not been triaged by the area owner label Jul 17, 2020
@AndyAyersMS AndyAyersMS added this to the Future milestone Jul 17, 2020
@BruceForstall BruceForstall added the JitUntriaged CLR JIT issues needing additional triage label Oct 28, 2020
@BruceForstall BruceForstall removed the JitUntriaged CLR JIT issues needing additional triage label Nov 7, 2020
@hez2010
Copy link
Contributor

hez2010 commented Feb 7, 2025

This has been addressed by #110827 and we should close it. (Perhaps update the todo in #7541 as all)

@AndyAyersMS
Copy link
Member

Yep.

This was likely "fixed" by dynamic PGO a while back, eg

Runtime=.NET 9.0

Method Job Toolchain Mean Error StdDev Median Ratio RatioSD
Combined Job-JXEEPS net10.0 0.0007 ns 0.0027 ns 0.0026 ns 0.0000 ns 0.04 0.12
Combined Job-SGEPJK net9.0 0.0207 ns 0.0032 ns 0.0027 ns 0.0214 ns 1.02 0.18
Separate Job-JXEEPS net10.0 0.0240 ns 0.0165 ns 0.0138 ns 0.0200 ns ? ?
Separate Job-SGEPJK net9.0 0.0019 ns 0.0061 ns 0.0051 ns 0.0000 ns ? ?
JustMe Job-JXEEPS net10.0 0.0000 ns 0.0000 ns 0.0000 ns 0.0000 ns 0.000 0.00
JustMe Job-SGEPJK net9.0 0.0301 ns 0.0199 ns 0.0176 ns 0.0230 ns 1.511 1.82

but if you disable tiered compilation:

EnvironmentVariables=DOTNET_TieredCompilation=0 Runtime=.NET 9.0

Method Job Toolchain Mean Error StdDev Median Ratio RatioSD
Combined Job-BRSGZU net10.0 0.0194 ns 0.0013 ns 0.0010 ns 0.0194 ns 0.004 0.00
Combined Job-DNGAOV net9.0 4.9356 ns 0.1226 ns 0.1147 ns 4.8767 ns 1.000 0.03
Separate Job-BRSGZU net10.0 0.0179 ns 0.0031 ns 0.0026 ns 0.0173 ns 0.004 0.00
Separate Job-DNGAOV net9.0 4.9616 ns 0.0981 ns 0.0870 ns 4.9532 ns 1.000 0.02
JustMe Job-BRSGZU net10.0 0.0229 ns 0.0117 ns 0.0098 ns 0.0180 ns 1.25 0.59
JustMe Job-DNGAOV net9.0 0.0197 ns 0.0083 ns 0.0069 ns 0.0170 ns 1.08 0.44

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
Projects
None yet
Development

No branches or pull requests

5 participants