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: enable GDV when static class deduction fails. #55660

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

AndyAyersMS
Copy link
Member

Update the early bailout in impDevirtualizeCall that gives up if
the object class cannot not be determined to check if we can do
GDV (guarded devirtualization) based on profile data.

Fixes one of the cases noted in #55079.

Update the early bailout in impDevirtualizeCall that gives up if
the object class cannot not be determined to check if we can do
GDV (guarded devirtualization) based on profile data.

Fixes one of the cases noted in dotnet#55079.
@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 Jul 14, 2021
@AndyAyersMS
Copy link
Member Author

@EgorBo PTAL
cc @dotnet/jit-contrib

On the second test case from #55079:

complus_jitprintdevirtualizedmethods=*
complus_noguionassert=0
complus_tieredpgo=1

corerun.exe ex.exe
Devirtualized virtual call to AFactory:GetA; now direct call to BFactory:GetA [exact]
Devirtualized virtual call to A:GetValue; now direct call to B:GetValue [exact]

and in the jitdump:

impDevirtualizeCall: no type available (op=LCL_VAR)
Considering guarded devirtualization at IL offset 35 (0x23)
Likely class for 0000000000000000 (unknown) is 00007FF93A546AC0 (B) [likelihood:100 classes seen:1]
virtual call would invoke method GetValue
Marking call [000019] as guarded devirtualization candidate; will guess for class B
...
lvaGrabTemp returning 10 (V10 tmp8) (a long lifetime temp) called for guarded devirt this exact temp.

lvaSetClass: setting class for V10 to (00007FF93A546AC0) B  [exact]
Direct call [000086] in block BB11

impDevirtualizeCall: Trying to devirtualize virtual call:
    class for 'this' is B [exact] (attrib 20000000)
    base method is A::GetValue
    devirt to B::GetValue -- exact
               [000086] I-C-G-------              *  CALLV vt-ind int    A.GetValue
               [000088] ------------ this in rcx  \--*  LCL_VAR   ref    V10 tmp8         
    exact; can devirtualize
... after devirt...
               [000086] I-C-G-------              *  CALL nullcheck int    B.GetValue
               [000088] ------------ this in rcx  \--*  LCL_VAR   ref    V10 tmp8         
Devirtualized virtual call to A:GetValue; now direct call to B:GetValue [exact]

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Jul 14, 2021

SPMI only sees one method with a diff.

         137 ( 8.00% of base) : 297269.dasm - System.Net.NTAuthentication:GetOutgoingBlob(System.Byte[],bool,byref):System.Byte[]:this

There were quite a few replay failures (not surprising) so the actual set of diffs is perhaps considerably larger. Not sure how to assess this without merging this and getting a new collection, then undoing this.

For the asp.net collection (~45K methods) there were ~1900 replay failures. Not currently easy to verify these are all caused by the above, but seems plausible.

@AndyAyersMS
Copy link
Member Author

PMI diffs didn't spot anything either, but both diff and base jits failed in SPC where we probably have the most relevant PGO data.

At any rate I think this change is clearly improved functionality and am not too worried about bad diffs.

@EgorBo
Copy link
Member

EgorBo commented Jul 14, 2021

Nice! I actually also tried to fix that via "get class handle for method" but didn't finish it

@AndyAyersMS
Copy link
Member Author

@EgorBo you might try running this over TE, I think your scripting is better than mine.

@EgorBo
Copy link
Member

EgorBo commented Jul 14, 2021

@EgorBo you might try running this over TE, I think your scripting is better than mine.

Sure, I've just started a couple of benchmarks to compare

@AndyAyersMS
Copy link
Member Author

Not obvious how/if the installer test failure (Breadcrumb_thread_does_not_finish_when_app_has_unhandled_exception) is related. Suspect it isn't...

@AndyAyersMS
Copy link
Member Author

Pretty sure failures are unrelated, have kicked off retries but CI doesn't seem to responding. Will look again in the morning.

@AndyAyersMS AndyAyersMS merged commit bd9d4e0 into dotnet:main Jul 15, 2021
@AndyAyersMS AndyAyersMS deleted the EnableGDVForUnknownClassCases branch July 15, 2021 14:51
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 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