Use a more exact base class for considerGuardedDevirtualization #108497
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
/azp run runtime-nativeaot-outerloop, runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
I ran a rt-sz run on this: MichalStrehovsky/rt-sz#74 (comment) This is a size regression but for good reasons. You can grab the before/after binaries from here: https://github.com/MichalStrehovsky/rt-sz/actions/runs/11158038241 I fed the webapiaot to Ghidra and then exported the analysis to BinDiff format and ran BinDiff on it to spot check the diffs. They are in the expected two categories - no longer devirtualizing things that were apparently impossible, and newly devirtualizing things that weren't. The newly devirtualized cases look interesting. For example, I'm looking at a devirtualized If you want to have a look, I'm putting the binexport files here: You shouldn't need Ghidra for this, just download the latest BinDiff and these two binexport files and create a new diff from them. |
|
@MichalStrehovsky thanks for looking into diffs! took a quick look at the diffs in BinDiff (TIL) 🙂 I was not able to detect any impact on CoreCLR as expected (presumably) since PGO tells us exactly what to choose and we never omit GDV checks for CoreCLR. PTAL @AndyAyersMS @dotnet/jit-contrib |
src/coreclr/jit/importercalls.cpp
Outdated
| @@ -8175,7 +8175,13 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, | |||
| return; | |||
| } | |||
|
|
|||
| considerGuardedDevirtualization(call, ilOffset, isInterface, baseMethod, baseClass, pContextHandle); | |||
| CORINFO_CLASS_HANDLE moreExactBaseClass = baseClass; | |||
| if (objClass != NO_CLASS_HANDLE) | |||
There was a problem hiding this comment.
Don't we already know objClass is not null here?
There was a problem hiding this comment.
Don't we already know
objClassis not null here?
Yep, you're correct, removed the condition. Looks like it's not needed to do isMoreSpecific here (and it won't work for the Michal's case since one of the classes doesn't exist in fact).
|
PGO stress failures look like #108360 |
|
Do we need to backport this? |
It is a missed optimization, so I would guess doesn't meet the bar. |
An attempt to address #108471 (comment)