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

Inliner: new observations (don't impact inlining for now) #53670

Merged
merged 29 commits into from
Jun 9, 2021

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jun 3, 2021

Extracted from #52708
Without changes which impact inlining decisions.

Empty jit diffs (as expected).

@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 Jun 3, 2021
src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved
@EgorBo EgorBo force-pushed the inliner-improvements-noperf branch from da5d989 to d12eed8 Compare June 3, 2021 12:31
@EgorBo
Copy link
Member Author

EgorBo commented Jun 3, 2021

@dotnet/jit-contrib @AndyAyersMS Please, take a look.
These changes don't affect codegen, just preparations for #52708

src/coreclr/jit/inline.cpp Outdated Show resolved Hide resolved
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the inline policies that derive from the DefaultPolicy should probably also implement the new per-policy xml dumping methods (invoking the parent version, then adding their own unique entries at the end).

Previously I'd dumped the data as a separate side file; keeping it inline with the xml makes sense. It feels like there might be cleaner ways to implement all this but I'm not sure it is worth the trouble. Better to focus on what we do with the data.

src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/inline.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/inline.cpp Outdated Show resolved Hide resolved
@EgorBo
Copy link
Member Author

EgorBo commented Jun 7, 2021

@AndyAyersMS could you please re-review?
NOTE: some observations aren't wired up in the PR, because in #52708 I had to re-write fgFindJumpTargets for them and I didn't want to do it in this PR.

Some of the inline policies that derive from the DefaultPolicy should probably also implement the new per-policy xml dumping methods (invoking the parent version, then adding their own unique entries at the end).
Previously I'd dumped the data as a separate side file; keeping it inline with the xml makes sense. It feels like there might be cleaner ways to implement all this but I'm not sure it is worth the trouble. Better to focus on what we do with the data.

Do you mind if I leave it as is for now, the per-policy XML stuff is not included by default if DOTNET_JitInlinePolicyDumpXml=1 is not set so it shouldn't affect any of your tools. It's just easier for me to validate effects of benefit multipliers on a collected data. Once I improve the DefaultPolicy I'll try to improve others, e.g. I'd love to re-calculate Model for ModelPolicy using these new observations.

I'm currently rewriting #52708 to be based on this PR

@EgorBo EgorBo force-pushed the inliner-improvements-noperf branch from 39825f9 to c2a94a2 Compare June 7, 2021 17:29
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind if I leave it as is for now, the per-policy XML stuff is not included by default

Sure, we can fix this later.

src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/inline.def Outdated Show resolved Hide resolved
@EgorBo
Copy link
Member Author

EgorBo commented Jun 8, 2021

@AndyAyersMS could you please re-review, I've addressed your feedback.
Also, I checked that SuperPMI returns empty diffs as expected.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Let's just make sure we get the right check for generic methods.

src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
@EgorBo
Copy link
Member Author

EgorBo commented Jun 8, 2021

A test for generics:

using System;

class Program
{
    static void Main()
    {
        Console.WriteLine(GenericClass<string>.Case1(1));
        Console.WriteLine(GenericClass<string>.Case2(1));
        Console.WriteLine(GenericClass<long>.Case1(1));
        Console.WriteLine(GenericClass<long>.Case2(1));
        Console.WriteLine(NonGenericClass.Case3<int>(1));
        Console.WriteLine(NonGenericClass.Case3<string>(""));
    }
}

public class GenericClass<T>
{

    // Case1: class is a [shared] generic, method doesn't use T
    public static int Case1(int a)
    {
        if (a == 42)
            throw new Exception("hello"); // some random code to exceed ALWAYS_INLINE limit
        return 0;
    }

    // Case2: class is a [shared] generic, method uses T
    public static int Case2(int a)
    {
        if (typeof(T) == typeof(float))
            throw new Exception("hello"); // some random code to exceed ALWAYS_INLINE limit
        return 0;
    }
}

public class NonGenericClass
{
    // Case3: class is not generic, method is
    public static int Case3<T>(T a)
    {
        if (a is float)
            throw new Exception("hello"); // some random code to exceed ALWAYS_INLINE limit
        return 0;
    }
}

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think the interesting case is when caller is not shared (generic or no) and callee is shared, but let's see how this plays out when you add heuristics based on these observations.

@EgorBo
Copy link
Member Author

EgorBo commented Jun 9, 2021

I still think the interesting case is when caller is not shared (generic or no) and callee is shared, but let's see how this plays out when you add heuristics based on these observations.

Will check that case separately too

@EgorBo EgorBo merged commit 968532c into dotnet:main Jun 9, 2021
@AndyAyersMS AndyAyersMS mentioned this pull request Jul 6, 2021
54 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Jul 9, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants