- Notifications
You must be signed in to change notification settings - Fork5.1k
Fold always false type checks#99761
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
If we have a program like:```csharpclass Never { }class Program{ static void Main() { Test(null); } [MethodImpl(MethodImplOptions.NoInlining)] static void Test(object o) { if (o is Never) Console.WriteLine("Hello!"); if (o.GetType() == typeof(Never)) Console.WriteLine("Hello!"); }}```We know these checks are never going to be true thanks to the whole program view. Fold these to `false`.Closes#97601 I presume? |
Uh oh!
There was an error while loading.Please reload this page.
/azp run runtime-nativeaot-outerloop |
| Azure Pipelines successfully started running 1 pipeline(s). |
Does this optimization kick in for interfaces? If so, can we add a test around IDynamicInterfaceCastable to ensure that we don't fold casts to types that have the DynamicCastableImplementationAttribute or interfaces they implement (as its possible for no types to statically implement these interfaces but casts to them to still succeed)? |
@EgorBo@jakobbotsch either of your could psychically debug the reason for hitting a |
Uh oh!
There was an error while loading.Please reload this page.
No good guess from my side without the jitdump, I'm afraid. |
It does kick in for interfaces. I cannot come up with a targeted test for this in regards to this optimization. I tried writing one, and ended up with exactly the same thing we already test for IDynamicInterfaceCastable. |
hm.. I tried to repro it locally on win-x64 using the CI steps and it succesfully completed on your branch, let's see what your new commit will show |
Yeah, the same happened to me. I hope we don't have nondeterminism :( |
/azp run runtime-nativeaot-outerloop |
| Azure Pipelines successfully started running 1 pipeline(s). |
@MichalStrehovsky I suspect you need to change return value in |
Good point. I'll let the nativeaot outerloop finish and change it tomorrow together with the JitInterface guid. |
It's not nondeterministic - the assert is hitting in main. It very likely broke sometime after136b100 because that's where my branch was when neither me nor Egor could repro the assert locally. I'll grab a jitdump and file is as a bug, it's unrelated to this PR. |
This is ready for review. @dotnet/ilc-contrib could someone have a look at the ILC part?@EgorBo could you have a look at the JIT part? |
/azp run runtime-nativeaot-outerloop |
| Azure Pipelines successfully started running 1 pipeline(s). |
Sigh, I think I have a fix for the SPMI failures but want to wait until nativeaot outerloop finishes. |
Uh oh!
There was an error while loading.Please reload this page.
| #if !READYTORUN | ||
| if (result == TypeCompareState.May | ||
| && (canNeverHaveInstanceOfSubclassOf(type1) || canNeverHaveInstanceOfSubclassOf(type2))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
CancompareTypesForEquality operate on types that are not allocated, but otherwise exist in the system? This change seems to make hidden assumptions about how this API is used by the JIT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll think about this some more. Worst case I'll just pull this part out. I already used up my JitInterface update quota for the month.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It would not be a problem today but it adds a hidden assumption. Removed it. I'll measure how much it impacts things and whether we want to track that as a separate bug.
I know the typeof optimization would help CsWinRT but dont' have numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We can fix that by adding abool flag that says the query is specifically forSystem.Typeoperator==/!= and only execute this logic for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It would not be a problem today but it adds a hidden assumption. Removed it. I'll measure how much it impacts things and whether we want to track that as a separate bug.
I removed the wrong part. Fixed it in the latest iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The JIT part specifically LGTM. There is also one use ofgetExactClasses in the helperexpansion phase, but that one doesn't need any treatment.
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
| @@ -2248,14 +2248,42 @@ private void getFieldInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_MET | |||
| // and STS::AccessCheck::CanAccess. | |||
| } | |||
| private bool canNeverHaveInstanceOfSubclassOf(TypeDesc type) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| privateboolcanNeverHaveInstanceOfSubclassOf(TypeDesctype) | |
| privateboolCanNeverHaveInstanceOfSubclassOf(TypeDesctype) |
Nit: We seem to use regular naming convention in this file for methods that are not directly exposed on JIT/EE interface
| public override bool CanReferenceConstructedMethodTable(TypeDesc type) | ||
| => _constructedMethodTables.Contains(type); | ||
| public override bool CanTypeOrCanonicalFormOfTypeBeAllocated(TypeDesc type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These two methods do very similar things, but they have very different names. Should these two methods have similar names, e.g.CanReferenceConstructedMethodTable +CanReferenceConstructedTypeOrCanonicalFormOfType;CanTypeBeAllocated +CanTypeOrCanonicalFormOfTypeBeAllocated ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM otherwise. Thanks!
/azp run runtime-nativeaot-outerloop |
| Azure Pipelines successfully started running 1 pipeline(s). |
Uh oh!
There was an error while loading.Please reload this page.
Fixes#97601
If we have a program like:
We know these checks are never going to be true thanks to the whole program view. Fold these to
false.