JIT: preliminaries to improve types #10172
Conversation
Refactor the logic in `impDevirtualizeCall` so that the part that determines the type of a tree for ref types is now a new utility method that can be called elsewhere. Update the utility to examine calls more closely. For inline candidates that return shared types, try and use the context to get to the unshared version of the type. For calls that are not inline candidates, look at the type in the signature available to the jit w/o context. Call the utility when we've created a temp for an inlinee's argument and the argument is not modified in the inlinee body. If we already thought we knew the type of the temp exactly, ensure that this new information agrees. Rework the logic in `impDevirtualizeCall` in anticipation of interface call devirtualization. Update the diagnostic stream to indicate the kind of call devirtualized nd the primary reason why devirtualization happened. Avoid fetching class and method names unless they're going to be used. Likewise try not to fetch attributes if we already have them on hand.
|
@JosephTremoulet PTAL Impacts about 80 methods in the jit-diff set. Has more impact when jitting, since many interesting shared methods are not inline candidates in R2R mode. For instance when jitting we can now devirtualize a call whose object is the result the List.get_Item. |
| if (lvaTable[tmpNum].lvClassIsExact) | ||
| { | ||
| assert(isExact); | ||
| assert(refClassHandle == lvaTable[tmpNum].lvClassHnd); |
There was a problem hiding this comment.
How do we know the new info won't be compatible but less precise than the old info?
There was a problem hiding this comment.
This is a bit of future proofing.
Currently the argument temps -- created over in impInlineFetchArg -- don't get any class/exact info. If/when they do, we will type them based on the method signature.
Any correct program will pass an argument value with the signature type or some subtype. So the argument type should be as good or better a bound on the type than the signature type.
In the update case, it might be worth checking for proper subtype relationship here, though if we somehow "downgrade" the type to a superclass, we still have a safeguard in the VM side of devirtualization, which also does this check.
src/jit/gentree.cpp
Outdated
| // otherwise the class handle. | ||
| // isExact set true if tree type is known to be exactly the handle type, | ||
| // otherwise actual type may be a subtype. | ||
| // isNonNull set truee if tree value is known not to be null, |
| CORINFO_CLASS_HANDLE objClass = gtGetClassHandle(thisObj, &isExact, &objIsNonNull); | ||
|
|
||
| // Bail if we know nothing. | ||
| if (objClass == nullptr) |
There was a problem hiding this comment.
How common is this case? Worth moving the check up to before fetching baseClass/baseClassAttribs?
There was a problem hiding this comment.
Makes sense to reorder and check for objType first, since we only get a type maybe 50% of the time. Will update.
| // Bail if devirt is disabled. | ||
| if (JitConfig.JitEnableDevirtualization() == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| const bool doPrint = JitConfig.JitPrintDevirtualizedMethods() == 1; |
There was a problem hiding this comment.
I'm confused what you're trying to achieve with doPrint in release builds here... looks like you only fill in the info strings under #if DEBUG, so release would just print the placeholders and the only info you really get is the total number of devirtualized calls per kind, right? If we're going to want that information from release builds, would it be better to fire an ETW event or something?
There was a problem hiding this comment.
doPrint only exists in DEBUG. There is no release mode diagnostic stream for devirtualization.
Not sure if devirtualization warrants an VM/ETW callback/event. Inlining needs it because other runtime subsystems are sensitive to inlining -- for instance rejitting needs to know what methods have been inlined.
If we do add a notification mechanism it will traffic only in handles and string literals to avoid any need for dynamic string allocation.
There was a problem hiding this comment.
Right, thanks, I was just misreading the code there, I like it how you have it.
| // Bail if devirt is disabled. | ||
| if (JitConfig.JitEnableDevirtualization() == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| const bool doPrint = JitConfig.JitPrintDevirtualizedMethods() == 1; |
There was a problem hiding this comment.
Right, thanks, I was just misreading the code there, I like it how you have it.
|
Pushed an update that fixes the typo and reorders the checks -- also fixed a dump mode AV and tweaked dump mode formatting a bit. |
|
Windows debug timed out in one of the chaos tests -- can't repro locally, so am going to retry. @dotnet-bot retest Windows_NT x64 Debug Build and Test |
|
OSX failure is a known issue, so will ignore it. |
Refactor the logic in `impDevirtualizeCall` so that the part that determines the type of a tree for ref types is now a new utility method that can be called elsewhere. Update the utility to examine calls more closely. For inline candidates that return shared types, try and use the context to get to the unshared version of the type. For calls that are not inline candidates, look at the type in the signature available to the jit w/o context. Call the utility when we've created a temp for an inlinee's argument and the argument is not modified in the inlinee body. If we already thought we knew the type of the temp exactly, ensure that this new information agrees. Rework the logic in `impDevirtualizeCall` in anticipation of interface call devirtualization. Update the diagnostic stream to indicate the kind of call devirtualized and the primary reason why devirtualization happened. Avoid fetching class and method names unless they're going to be used. Likewise try not to fetch attributes if we already have them on hand.
Refactor the logic in
impDevirtualizeCallso that the part that determinesthe type of a tree for ref types is now a new utility method that can be called
elsewhere.
Update the utility to examine calls more closely. For inline candidates that
return shared types, try and use the context to get to the unshared version of the
type. For calls that are not inline candidates, look at the type in the signature
available to the jit w/o context.
Call the utility when we've created a temp for an inlinee's argument and the
argument is not modified in the inlinee body. If we already thought we knew the
type of the temp exactly, ensure that this new information agrees.
Rework the logic in
impDevirtualizeCallin anticipation of interface calldevirtualization. Update the diagnostic stream to indicate the kind of call
devirtualized nd the primary reason why devirtualization happened.
Avoid fetching class and method names unless they're going to be used. Likewise
try not to fetch attributes if we already have them on hand.