Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
EgorBo merged 29 commits intodotnet:mainfromEgorBo:inliner-improvements-noperf
Jun 9, 2021

Conversation

@EgorBo
Copy link
Member

@EgorBoEgorBo commentedJun 3, 2021
edited
Loading

Extracted from#52708
Without changes which impact inlining decisions.

Empty jit diffs (as expected).

@ghostghost added the area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labelJun 3, 2021
@EgorBoEgorBoforce-pushed theinliner-improvements-noperf branch fromda5d989 tod12eed8CompareJune 3, 2021 12:31
@EgorBo
Copy link
MemberAuthor

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

Copy link
Member

@AndyAyersMSAndyAyersMS 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 theDefaultPolicy 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.

@EgorBo
Copy link
MemberAuthor

EgorBo commentedJun 7, 2021
edited
Loading

@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 ifDOTNET_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

@EgorBoEgorBoforce-pushed theinliner-improvements-noperf branch from39825f9 toc2a94a2CompareJune 7, 2021 17:29
Copy link
Member

@AndyAyersMSAndyAyersMS 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.

EgorBoand others added2 commitsJune 8, 2021 16:54
Co-authored-by: Andy Ayers <andya@microsoft.com>
@EgorBo
Copy link
MemberAuthor

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

Copy link
Member

@AndyAyersMSAndyAyersMS 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.

@EgorBo
Copy link
MemberAuthor

EgorBo commentedJun 8, 2021
edited
Loading

A test for generics:

usingSystem;classProgram{staticvoidMain(){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>(""));}}publicclassGenericClass<T>{// Case1: class is a [shared] generic, method doesn't use TpublicstaticintCase1(inta){if(a==42)thrownewException("hello");// some random code to exceed ALWAYS_INLINE limitreturn0;}// Case2: class is a [shared] generic, method uses TpublicstaticintCase2(inta){if(typeof(T)==typeof(float))thrownewException("hello");// some random code to exceed ALWAYS_INLINE limitreturn0;}}publicclassNonGenericClass{// Case3: class is not generic, method ispublicstaticintCase3<T>(Ta){if(aisfloat)thrownewException("hello");// some random code to exceed ALWAYS_INLINE limitreturn0;}}

Copy link
Member

@AndyAyersMSAndyAyersMS 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
MemberAuthor

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

@EgorBoEgorBo merged commit968532c intodotnet:mainJun 9, 2021
@AndyAyersMSAndyAyersMS mentioned this pull requestJul 6, 2021
54 tasks
@ghostghost locked asresolvedand limited conversation to collaboratorsJul 9, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

No one assigned

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@EgorBo@AndyAyersMS

[8]ページ先頭

©2009-2025 Movatter.jp