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

Convert isExactType check to JIT/EE interface call #97424

Merged
merged 4 commits into from
Jan 24, 2024

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Jan 23, 2024

The approximation of isExactType from class flags had false positives (correctness) and false negatives issues. Converting it to JIT/EE interface method fixes them both.

Fixes #97134

The approximation of isExactType from class flags had false positives (correctness) and false negatives issues. Converting it to JIT/EE interface method fixes them both.

Fixes dotnet#97134
@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 Jan 23, 2024
@ghost ghost assigned jkotas Jan 23, 2024
@ghost
Copy link

ghost commented Jan 23, 2024

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

The approximation of isExactType from class flags had false positives (correctness) and false negatives issues. Converting it to JIT/EE interface method fixes them both.

Fixes #97134

Author: jkotas
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jkotas
Copy link
Member Author

jkotas commented Jan 23, 2024

cc @dotnet/jit-contrib

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a coincidence, but in https://github.com/dotnet/runtime/pull/97387/files I currently have a failing test that fails because IsExactClass returns true for a class with [TypeIdentifier] and I assume this PR will fix it. The reason current jit's cast expansion logic is not hitting this issue becuase it doesn't try to expand CORINFO_HELP_ISINSTANCEOFANY for exact classes

@EgorBo
Copy link
Member

EgorBo commented Jan 23, 2024

Does IsExactType return true for int[]? I assume no and wonder if it results in some jit-diffs (we can run MihuBot once PR passes CI)

@jkotas
Copy link
Member Author

jkotas commented Jan 23, 2024

Does IsExactType return true for int[]?

It does not and it cannot not. int[], uint[] and arrays of int/uint backed enums are interchangeable.

@jkotas
Copy link
Member Author

jkotas commented Jan 23, 2024

I assume no and wonder if it results in some jit-diffs

The JIT approximation returned false for int[] as well.

I agree that it is interesting to get jit-diff.

@EgorBo
Copy link
Member

EgorBo commented Jan 23, 2024

There is also getExactClasses API which is implemented only for NativeAOT at the moment - I wonder if it doesn't respect type variance too. JIT might use that API to optimize casts and it might skip the fallback call because it assumes it's the only type that can pass the cast.

@jkotas
Copy link
Member Author

jkotas commented Jan 24, 2024

JIT might use that API to optimize casts and it might skip the fallback call

Do you mean this place

(info.compCompHnd->getExactClasses(pResolvedToken->hClass, 1, &actualImplCls) == 1) &&
(actualImplCls != NO_CLASS_HANDLE) && impIsClassExact(actualImplCls))
? The code checks whether the implementation is actually exact. I do not see a problem there. (There may be a naming confusion - getExactClasses vs. isExactType do not use "exact" to mean the same thing.)

@EgorBo
Copy link
Member

EgorBo commented Jan 24, 2024

JIT might use that API to optimize casts and it might skip the fallback call

Do you mean this place

(info.compCompHnd->getExactClasses(pResolvedToken->hClass, 1, &actualImplCls) == 1) &&
(actualImplCls != NO_CLASS_HANDLE) && impIsClassExact(actualImplCls))

? The code checks whether the implementation is actually exact. I do not see a problem there. (There may be a naming confusion - getExactClasses vs. isExactType do not use "exact" to mean the same thing.)

Yes, I meant that case - I agree it's ok since it checks isExact explicitly 👍

There may be a naming confusion

Agree, getExactClasses was named like that as an opposite to getLikelyClasses

@EgorBo
Copy link
Member

EgorBo commented Jan 24, 2024

@MihuBot

@EgorBo
Copy link
Member

EgorBo commented Jan 24, 2024

@jkotas an unrelated question:

Currently, for

var result = (MySealedClass)obj;

jit emits something like this:

if (obj.GetType() == typeof(MySealedClass))
    result = obj;
else
   CORINFO_HELP_CHKCASTCLASS(obj, MySealedClass)
   ret3 // always throw anyway

Can we hit here the same issue we hit recently with version bubbles in R2R if MySealedClass becomes non-sealed in a new version? or isExactClass won't return true for it even if has sealed?

@jkotas
Copy link
Member Author

jkotas commented Jan 24, 2024

isExactClass won't return true for it even if has sealed?

Yes. IsEffectivelySealed for R2R returns true only if the type is in the same version bubble:

public override bool IsEffectivelySealed(TypeDesc type)
{
return _compilationModuleGroup.VersionsWithType(type) && base.IsEffectivelySealed(type);
}

All implementations return the same constant. Unlikely to be needed again.
@jkotas
Copy link
Member Author

jkotas commented Jan 24, 2024

/azp run runtime-coreclr outerloop, runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@EgorBo
Copy link
Member

EgorBo commented Jan 24, 2024

No jit-diffs MihuBot/runtime-utils#228 (comment)

@EgorBo
Copy link
Member

EgorBo commented Jan 24, 2024

the build-in-progress jobs seem to stuck? 10h for the NativeAOT one

@jkotas
Copy link
Member Author

jkotas commented Jan 24, 2024

the build-in-progress jobs seem to stuck? 10h for the NativeAOT one

Received request to deprovision: The request was cancelled by the remote provider.

These legs passed in previous runs. I do not think we need to retry.

@jkotas jkotas merged commit 95bf3d9 into dotnet:main Jan 24, 2024
121 of 125 checks passed
@jkotas jkotas deleted the isexacttype branch January 24, 2024 17:19
@github-actions github-actions bot locked and limited conversation to collaborators Feb 24, 2024
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.

JIT does not recognize typeof(X) == o.GetType() checks as intrinsics with R2R
2 participants