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

JIT: Fold more always failing type checks #97005

Closed
wants to merge 2 commits into from

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Jan 15, 2024

Folding x isinst B where x is statically of type A is currently done via a call to compareTypesForCast. If it returns TypeCompareState::Must then we know that A can be cast to B, which also means any class derived from A can be cast to B, so we can always fold that. However, if it returns TypeCompareState::MustNot we only learn something if we know x is exactly A; otherwise, if x is more derived than A, it could still potentially be cast to B.

In cases where the type hierarchies are disjoint, however, we can still know that some type checks will fail. To detect the case add a query in the opposite direction; that is, if B cannot be cast to A, then the type hierarchies are disjoint, so in that case we can fold it, regardless of having exact knowledge. This also requires some explicit checks for interface types, but perhaps we can move those to the EE side.

Fix #97000

[MethodImpl(MethodImplOptions.NoInlining)]
static bool Test(List<int> x) => Foo(x);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
static bool Foo<TSource>(IEnumerable<TSource> x) => x is TSource[];

Before:

G_M10367_IG01:  ;; offset=0x0000
       4883EC28             sub      rsp, 40
						;; size=4 bbWeight=1 PerfScore 0.25

G_M10367_IG02:  ;; offset=0x0004
       488BD1               mov      rdx, rcx
       48B9E88DCCB6FB7F0000 mov      rcx, 0x7FFBB6CC8DE8      ; int[]
       E8EAF6165E           call     CORINFO_HELP_ISINSTANCEOFARRAY
       4885C0               test     rax, rax
       0F95C0               setne    al
       0FB6C0               movzx    rax, al
						;; size=27 bbWeight=1 PerfScore 3.00

G_M10367_IG03:  ;; offset=0x001F
       4883C428             add      rsp, 40
       C3                   ret      
						;; size=5 bbWeight=1 PerfScore 1.25
; Total bytes of code: 36

After:

G_M10367_IG02:  ;; offset=0x0000
       33C0                 xor      eax, eax
						;; size=2 bbWeight=1 PerfScore 0.25

G_M10367_IG03:  ;; offset=0x0002
       C3                   ret      
						;; size=1 bbWeight=1 PerfScore 1.00
; Total bytes of code: 3

Folding `x isinst B` where `x` is statically of type `A` is currently
done via a call to `compareTypesForCast`. If it returns
`TypeCompareState::Must` then we know that `A` can be cast to `B`, which
also means any class derived from `A` can be cast to `B`, so we can
always fold that. However, if it returns `TypeCompareState::MustNot` we
only learn something if we know `x` is exactly `A`; otherwise, if `x` is
more derived than `A`, it could still potentially be cast to `B`.

In cases where the type hierarchies are distinct, however, we can still
know that some type checks will fail. To detect the case add a query in
the opposite direction; that is, if `B` cannot be cast to `A`, then the
type hierarchies are distinct, so in that case we can fold it,
regardless of having exact knowledge. This also requires some explicit
checks for interface types, but perhaps we can move those to the EE
side.

Fix dotnet#97000
@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 15, 2024
@ghost ghost assigned jakobbotsch Jan 15, 2024
@ghost
Copy link

ghost commented Jan 15, 2024

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

Issue Details

Folding x isinst B where x is statically of type A is currently done via a call to compareTypesForCast. If it returns TypeCompareState::Must then we know that A can be cast to B, which also means any class derived from A can be cast to B, so we can always fold that. However, if it returns TypeCompareState::MustNot we only learn something if we know x is exactly A; otherwise, if x is more derived than A, it could still potentially be cast to B.

In cases where the type hierarchies are distinct, however, we can still know that some type checks will fail. To detect the case add a query in the opposite direction; that is, if B cannot be cast to A, then the type hierarchies are distinct, so in that case we can fold it, regardless of having exact knowledge. This also requires some explicit checks for interface types, but perhaps we can move those to the EE side.

Fix #97000

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch jakobbotsch changed the title JIT: Fold more always failing casts JIT: Fold more always failing type checks Jan 15, 2024
Comment on lines +5347 to +5349
// TODO: compareTypesForCast(interfaceType, otherType) returning
// MustNot seems less useful than just having it return May. If we
// change this on EE side we can avoid these getClassAttribs calls.
Copy link
Member Author

Choose a reason for hiding this comment

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

@jkotas do you think it makes sense to change this in compareTypesForCast? From what I understand, the current semantics of compareTypesForCast is "if I have an object of exact type A, can it be cast to type B" -- which is a meaningless question when A is an interface type. Alternatively, do you have any suggestions on better ways to check what we're after here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, looks like we fold Type.IsAssignableFrom/Type.IsAssignableTo using compareTypesForCast as well, so seems like we cannot really change its behavior.

Copy link
Member

Choose a reason for hiding this comment

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

do you think it makes sense to change this in compareTypesForCast?

Yes, I think this logic should be better done on the EE side.

looks like we fold Type.IsAssignableFrom/Type.IsAssignableTo using compareTypesForCast as well, so seems like we cannot really change its behavior.

You can add a flag to the compareTypesForCast or add a new JIT/EE interface method.

@jakobbotsch
Copy link
Member Author

@MihuBot

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr superpmi-diffs

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jakobbotsch
Copy link
Member Author

Diffs after a collection on this branch. Pretty minor overall and mostly in tests. Doesn't seem worth the throughput cost or complexity of changing the JIT-EE interface.

image

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.

Array variance checks are not optimized away for types known to be not arrays
2 participants