Uh oh!
There was an error while loading. Please reload this page.
Eliminate dead branches around typeof comparisons - #102248
Conversation
RyuJIT will already do dead branch elimination for `typeof(X) == typeof(Y)` patterns, but we couldn't do elimination around `foo == typeof(X)`. This fixes that using whole program knowledge - if we never saw a constructed `MT` for `X`, the comparison is not going to be true. Because it needs whole program, we still scan this dead branch so in the end this doesn't save much. We can eventually do better. I'm doing this in `SubstitutedILProvider` instead of in RyuJIT: this is because we currently only reap a small benefit from this optimization due to it only happening during compilation phase. We need to do this during scanning as well. I think I can extend it to scannig. But the extension will require the optimization to 100% guaranteed happen during codegen. We cannot rely on whether RyuJIT will feel like it. `SubstitutedILProvider` is our way to ensure the optimization will happen no matter what - the IL from the branch will be gone and RyuJIT can at most remove the comparison (we don't mind much if it's left).
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
MichalStrehovsky
commented
May 15, 2024
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| if (reader.ReadILOpcode() is not ILOpcode.callvirt and not ILOpcode.call) | ||
| return false; | ||
| // We don't actually mind if this is not Object.GetType |
There was a problem hiding this comment.
If it is an arbitrary call, can it return a type that happens to be equal to the other type?
Or is the idea that this case will fail the CanReferenceConstructedTypeOrCanonicalFormOfType check below? Ie the other argument can be anything. We are just skipping the specific common patterns here to keep things simple.
There was a problem hiding this comment.
Yes, we should be okay with any value loaded from a local or parameter. So also any value a method call could return.
We just don't have facilities to accept any value, so only a couple recognized patterns are allowed. Allowing any instance method call is less work than also checking if it's object.GetType.
| if (knownType.IsCanonicalDefinitionType(CanonicalFormKind.Any)) | ||
| return false; | ||
| if (_devirtualizationManager.CanReferenceConstructedTypeOrCanonicalFormOfType(knownType)) |
There was a problem hiding this comment.
Do we need to call convert ConvertToCanonForm before calling CanReferenceConstructedTypeOrCanonicalFormOfType? Or is the type guaranteed to be normalized somehow?
There was a problem hiding this comment.
Yes, we should convert to canon. Good catch.
jkotas
commented
May 17, 2024
I have run this under debugger on this simple test: I would expect the substitution to trigger for it, but it is not happening. It never hits breakpoint at this line https://github.com/dotnet/runtime/pull/102248/files#diff-7c5a8ad684ce4e7f583b3bc392219bb15fc17e8400b172a2ae32d5301b7cdd0bR1027 . Is that expected? |
MichalStrehovsky
commented
May 17, 2024
Yes, you need to flip them to the more common pattern. The problem is in the IL scanner. IL scanner only does the "downgrade result of runtime/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs Lines 872 to 897 in 4246ba1 We really need some better facilities to analyze IL in C#, but also I don't know if I want us to build a "proper" IL importer in C#. |
MichalStrehovsky
commented
May 17, 2024
(I plan to look into at least sharing this code between scanner and substitutions in some way.) |
jkotas
commented
May 17, 2024
Could you please share a program that hits this line https://github.com/dotnet/runtime/pull/102248/files#diff-7c5a8ad684ce4e7f583b3bc392219bb15fc17e8400b172a2ae32d5301b7cdd0bR1027 in the compiler? Or is this WIP and this path is not reachable yet? |
MichalStrehovsky
commented
May 17, 2024
It's the tests that are part of this PR. We also have hits in corelib, for example: (The above will also be a real saving once we can do this optimization in the scanner - this is the only places that boxes DateTime and Decimal and that's a 100 kB saving on an app that uses reflection. It doesn't kick in right now, because the scanner will see we box DateTime/decimal and that destroys our opportunity to get rid of it because |
jkotas
commented
May 17, 2024
I have extracted the test into a small program: I have compiled the test in release mode (the test is under It fails the pattern match in |
MichalStrehovsky
commented
May 17, 2024
Weird, I don't know how the test would pass without it. I've submitted #102374 with just the test because I don't want to switch branches locally right now. |
MichalStrehovsky
commented
May 17, 2024
The tests are all failing in #102374 so the optimization here works. I agree that for the local case this is pretty fragile. This is another case where the expectation is that this will mostly come from a parameter in real world code. Loading it from a local was just equally cheap in the pattern match so I just allowed it. But parameter is the main use case. |
I have figured out one of the mysteries: The dotnet/runtime build sets The ordinary user projects out there do not set |
Ok, this was the other part of the mystery. |
| { | ||
| Debug.Assert(type.NormalizeInstantiation() == type); | ||
| Debug.Assert(ConstructedEETypeNode.CreationAllowed(type)); | ||
| return _constructedMethodTables.Contains(type); |
There was a problem hiding this comment.
Should we also assert that we are only adding normalizations into _constructedMethodTables when it is populated?
This fixes the problem discussed at dotnet#102248 (comment). Now we call into the same code from both substitutions and scanner.
Before this PR, we were somewhat able to eliminate dead typeof checks such as:
```csharp
if (someType == typeof(Foo)
{
ExpensiveMethod();
}
```
This work was done in dotnet#102248.
However, the optimization only happened during codegen. This meant that when building the whole program view, we'd still look at `ExpensiveMethod` and whatever damage this caused to the whole program view was permanent.
With this PR, the scanner now becomes aware of the optimization we do during codegen and tries to defer injecting dependencies until we will need them.
With this change, we detect the conditional branch, and generate whatever dependencies from the basic block as conditional. That way scanning can fully skip scanning `ExpensiveMethod` and the subsequent optimization will ensure the missed scanning will not cause issues at codegen time.Before this PR, we were somewhat able to eliminate dead typeof checks such as:
```csharp
if (someType == typeof(Foo)
{
ExpensiveMethod();
}
```
This work was done in dotnet#102248.
However, the optimization only happened during codegen. This meant that when building the whole program view, we'd still look at `ExpensiveMethod` and whatever damage this caused to the whole program view was permanent.
With this PR, the scanner now becomes aware of the optimization we do during codegen and tries to defer injecting dependencies until we will need them.
With this change, we detect the conditional branch, and generate whatever dependencies from the basic block as conditional. That way scanning can fully skip scanning `ExpensiveMethod` and the subsequent optimization will ensure the missed scanning will not cause issues at codegen time.This fixes the problem discussed at dotnet#102248 (comment). Now we call into the same code from both substitutions and scanner.
Before this PR, we were somewhat able to eliminate dead typeof checks such as:
```csharp
if (someType == typeof(Foo)
{
ExpensiveMethod();
}
```
This work was done in dotnet#102248.
However, the optimization only happened during codegen. This meant that when building the whole program view, we'd still look at `ExpensiveMethod` and whatever damage this caused to the whole program view was permanent.
With this PR, the scanner now becomes aware of the optimization we do during codegen and tries to defer injecting dependencies until we will need them.
With this change, we detect the conditional branch, and generate whatever dependencies from the basic block as conditional. That way scanning can fully skip scanning `ExpensiveMethod` and the subsequent optimization will ensure the missed scanning will not cause issues at codegen time.Before this PR, we were somewhat able to eliminate dead typeof checks such as:
```csharp
if (someType == typeof(Foo)
{
ExpensiveMethod();
}
```
This work was done in dotnet#102248.
However, the optimization only happened during codegen. This meant that when building the whole program view, we'd still look at `ExpensiveMethod` and whatever damage this caused to the whole program view was permanent.
With this PR, the scanner now becomes aware of the optimization we do during codegen and tries to defer injecting dependencies until we will need them.
With this change, we detect the conditional branch, and generate whatever dependencies from the basic block as conditional. That way scanning can fully skip scanning `ExpensiveMethod` and the subsequent optimization will ensure the missed scanning will not cause issues at codegen time.Before this PR, we were somewhat able to eliminate dead typeof checks such as:
```csharp
if (someType == typeof(Foo)
{
ExpensiveMethod();
}
```
This work was done in dotnet#102248.
However, the optimization only happened during codegen. This meant that when building the whole program view, we'd still look at `ExpensiveMethod` and whatever damage this caused to the whole program view was permanent.
With this PR, the scanner now becomes aware of the optimization we do during codegen and tries to defer injecting dependencies until we will need them.
With this change, we detect the conditional branch, and generate whatever dependencies from the basic block as conditional. That way scanning can fully skip scanning `ExpensiveMethod` and the subsequent optimization will ensure the missed scanning will not cause issues at codegen time.This fixes the problem discussed at #102248 (comment). Now we call into the same code from both substitutions and scanner.
Before this PR, we were somewhat able to eliminate dead typeof checks such as:
```csharp
if (someType == typeof(Foo)
{
ExpensiveMethod();
}
```
This work was done in dotnet#102248.
However, the optimization only happened during codegen. This meant that when building the whole program view, we'd still look at `ExpensiveMethod` and whatever damage this caused to the whole program view was permanent.
With this PR, the scanner now becomes aware of the optimization we do during codegen and tries to defer injecting dependencies until we will need them.
With this change, we detect the conditional branch, and generate whatever dependencies from the basic block as conditional. That way scanning can fully skip scanning `ExpensiveMethod` and the subsequent optimization will ensure the missed scanning will not cause issues at codegen time.Before this PR, we were somewhat able to eliminate dead typeof checks such as:
```csharp
if (someType == typeof(Foo)
{
ExpensiveMethod();
}
```
This work was done in #102248.
However, the optimization only happened during codegen. This meant that when building the whole program view, we'd still look at `ExpensiveMethod` and whatever damage this caused to the whole program view was permanent.
With this PR, the scanner now becomes aware of the optimization we do during codegen and tries to defer injecting dependencies until we will need them.
With this change, we detect the conditional branch, and generate whatever dependencies from the basic block as conditional. That way scanning can fully skip scanning `ExpensiveMethod` and the subsequent optimization will ensure the missed scanning will not cause issues at codegen time.
RyuJIT will already do dead branch elimination for
typeof(X) == typeof(Y)patterns, but we couldn't do elimination aroundfoo == typeof(X). This fixes that using whole program knowledge - if we never saw a constructedMTforX, the comparison is not going to be true. Because it needs whole program, we still scan this dead branch so in the end this doesn't save much. We can eventually do better.I'm doing this in
SubstitutedILProviderinstead of in RyuJIT: this is because we currently only reap a small benefit from this optimization due to it only happening during compilation phase. We need to do this during scanning as well. I think I can extend it to scannig. But the extension will require the optimization to 100% guaranteed happen during codegen. We cannot rely on whether RyuJIT will feel like it.SubstitutedILProvideris our way to ensure the optimization will happen no matter what - the IL from the branch will be gone and RyuJIT can at most remove the comparison (we don't mind much if it's left).Cc @dotnet/ilc-contrib