Fold "cast(cast(obj, cls), cls)" to "cast(obj, cls)" #98337

EgorBo · 2024-02-13T01:32:58Z

Example:

static T Test<T>(object o) where T : class
{
    return o as T;
}

Current (Main) codegen:

; Assembly listing for method Program:Test[System.__Canon](System.Object)
       push     rbx
       sub      rsp, 48
       mov      qword ptr [rsp+0x28], rcx
       mov      rbx, rcx
       mov      rcx, qword ptr [rbx+0x38]
       mov      rcx, qword ptr [rcx]
       call     CORINFO_HELP_ISINSTANCEOFANY
       mov      r8, rax
       test     r8, r8
       je       SHORT G_M31297_IG05
       mov      rcx, qword ptr [rbx+0x38]
       mov      rcx, qword ptr [rcx]
       cmp      qword ptr [r8], rcx
       je       SHORT G_M31297_IG05
       mov      rdx, rax
       call     CORINFO_HELP_CHKCASTANY
       mov      r8, rax
G_M31297_IG05:
       mov      rax, r8
       add      rsp, 48
       pop      rbx
       ret      
; Total bytes of code 65

New (PR) codegen:

; Assembly listing for method Program:Test[System.__Canon](System.Object)
       sub      rsp, 40
       mov      qword ptr [rsp+0x20], rcx
       mov      rcx, qword ptr [rcx+0x38]
       mov      rcx, qword ptr [rcx]
       call     CORINFO_HELP_ISINSTANCEOFANY
       nop      
       add      rsp, 40
       ret      
; Total bytes of code 27

ghost · 2024-02-13T01:33:11Z

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

Issue Details

Example:

static T Test<T>(object o) where T : class
{
    return o as T;
}

Current (Main) codegen:

; Assembly listing for method Program:Test[System.__Canon](System.Object)
       push     rbx
       sub      rsp, 48
       mov      qword ptr [rsp+0x28], rcx
       mov      rbx, rcx
       mov      rcx, qword ptr [rbx+0x38]
       mov      rcx, qword ptr [rcx]
       call     CORINFO_HELP_ISINSTANCEOFANY
       mov      r8, rax
       test     r8, r8
       je       SHORT G_M31297_IG05
       mov      rcx, qword ptr [rbx+0x38]
       mov      rcx, qword ptr [rcx]
       cmp      qword ptr [r8], rcx
       je       SHORT G_M31297_IG05
       mov      rdx, rax
       call     CORINFO_HELP_CHKCASTANY
       mov      r8, rax
G_M31297_IG05:
       mov      rax, r8
       add      rsp, 48
       pop      rbx
       ret      
; Total bytes of code 65

New (PR) codegen:

; Assembly listing for method Program:Test[System.__Canon](System.Object)
       sub      rsp, 40
       mov      qword ptr [rsp+0x20], rcx
       mov      rcx, qword ptr [rcx+0x38]
       mov      rcx, qword ptr [rcx]
       call     CORINFO_HELP_ISINSTANCEOFANY
       nop      
       add      rsp, 40
       ret      
; Total bytes of code 27

Author:	EgorBo
Assignees:	EgorBo
Labels:	`area-CodeGen-coreclr`
Milestone:	-

src/coreclr/jit/assertionprop.cpp

…ookup

EgorBo · 2024-02-13T14:51:25Z

src/coreclr/jit/importer.cpp

@@ -1677,8 +1677,7 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken
        return slotPtrTree;
    }

-    slotPtrTree = gtNewIndir(TYP_I_IMPL, slotPtrTree, GTF_IND_NONFAULTING);
-    slotPtrTree->gtFlags &= ~GTF_GLOB_REF; // TODO-Bug?: this is a quirk. Can we mark this indirection invariant?
+    slotPtrTree = gtNewIndir(TYP_I_IMPL, slotPtrTree, GTF_IND_NONFAULTING | GTF_IND_INVARIANT);


I don't see why it can't be invariant (and removed the GTF_GLOB_REF quirk). However, I most likely will have to unmark it as invariant in my "inlining for shared generics" work for certain cases, but it's irrelevant here.

It was done for my snippet in the PR's description, otherwise two exactly the same runtime lookups had different conservative VNs.

I would double-check this with somebody on the runtime team; getting this wrong can have subtle consequences.

So from my understanding there are two types of runtime lookups:

Caller-side

Callee-side

The caller side is typically not invariant (at least in JIT) and requires a helper call + expansion machinery (checking for null/dictionary resize).

So if a runtime lookup doesn't need a helper-call/null check/size check - it's a fully invariant bunch of ind loads on top of TypeCtx.

It might be a bit more complicated if we implement inlining for shared generics where we're going to have both caller and callee lookups in the same method, but I will keep that in mind once I do that sort of inlining (as part of net9). Currently we never have such cases.

cc @jkotas to confirm.

The generic dictionary slots that do not require helper-call/null check/size check can be treated as invariant. This change looks fine to me.

So from my understanding there are two types of runtime lookups:
Caller-side
Callee-side

I am not sure what you mean. There is no distinction like this that I am aware of.

I am not sure what you mean. There is no distinction like this that I am aware of.

Yes, please, ignore that part. There are just 4 types of lookups:

Helper call + nullcheck + INDs (fast path)

Helper call + nullcheck + sizecheck + INDs (fast path)

~~nullcheck + INDs (fast path)~~

INDs (fast path)
where INDs is [0..n] nested indirect loads.

nullcheck + INDs (fast path)

Nit: These do not exist. The null check needs a helper call slow path.

EgorBo · 2024-02-13T17:30:42Z

@dotnet/jit-contrib PTAL, ready-for-review. Diffs - a small number of regressions from marking GT_RUNTIMELOOKUP as invariant + CSE.

Hopefully, it setups a handy place for other VN-based foldings.

src/coreclr/jit/gentree.cpp

AndyAyersMS · 2024-02-14T03:40:22Z

src/coreclr/jit/gentree.cpp

+//    The original tree wrapped with a COMMA node that contains the side effects
+//    or just the tree itself if sideEffectSource has no side effects.
+//
+GenTree* Compiler::gtWrapWithSideEffects(GenTree*     tree,


Seems like commonly tree is a subtree of sideEffectsSource.

If tree itself has side effects, do those get handled properly?

That is a good point, added a comment. Technically, it should be either a sideffect-free subtree of sideEffectsSource or pretty much any node with any sideffects if it's not a subtree. In two uses of this API where tree is a subnode of sideEffectsSource we do check it being side-effect free at the callsite. It's probably possible to be smarter here and extract the effects properly for any case, but not now. Setup-arg stuff in args makes it a bit more complicated 🙁

src/coreclr/jit/assertionprop.cpp

Co-authored-by: Andy Ayers <andya@microsoft.com>

src/coreclr/jit/gentree.cpp

src/coreclr/jit/assertionprop.cpp

EgorBo · 2024-02-14T14:50:54Z

@AndyAyersMS I've addressed your concerns and clarified the runtime lookup issue, this is ready for another look 🙂

ghost assigned EgorBo Feb 13, 2024

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 13, 2024

VN version

4383fba

EgorBo force-pushed the opt-cast-cast-lookup branch from 72d642c to 4383fba Compare February 13, 2024 03:21

EgorBo marked this pull request as ready for review February 13, 2024 11:03

EgorBo added 5 commits February 13, 2024 13:38

Clean up

9dec5cc

Clean up

04a82ee

Merge branch 'main' into opt-cast-cast-lookup

72817ff

Update assertionprop.cpp

fae7647

Clean up

76081b3

jakobbotsch reviewed Feb 13, 2024

View reviewed changes

src/coreclr/jit/assertionprop.cpp Outdated Show resolved Hide resolved

EgorBo added 2 commits February 13, 2024 15:21

Merge branch 'main' of github.com:dotnet/runtime into opt-cast-cast-l…

eff3ef8

…ookup

Address feedback

d5a7088

EgorBo commented Feb 13, 2024

View reviewed changes

EgorBo requested a review from AndyAyersMS February 13, 2024 20:53

AndyAyersMS reviewed Feb 14, 2024

View reviewed changes

Apply suggestions from code review

80a8e2c

Co-authored-by: Andy Ayers <andya@microsoft.com>

jakobbotsch reviewed Feb 14, 2024

View reviewed changes

src/coreclr/jit/gentree.cpp Outdated Show resolved Hide resolved

Cleanup

395c103

EgorBo force-pushed the opt-cast-cast-lookup branch from 36cb35a to 395c103 Compare February 14, 2024 11:12

EgorBo added 3 commits February 14, 2024 12:33

add a comment

c9bc1f4

add a todo

47a87d6

improve comment

a46c811

SingleAccretion reviewed Feb 14, 2024

View reviewed changes

src/coreclr/jit/gentree.cpp Outdated Show resolved Hide resolved

SingleAccretion reviewed Feb 14, 2024

View reviewed changes

src/coreclr/jit/assertionprop.cpp Outdated Show resolved Hide resolved

Address feedback

b4ab4c5

EgorBo mentioned this pull request Feb 15, 2024

BitCast<TYP_REF>(TypeHandleToRuntimeTypeHandle(clsHandle)) => nongc obj #97955

Merged

AndyAyersMS approved these changes Feb 15, 2024

View reviewed changes

EgorBo merged commit 0272fcc into dotnet:main Feb 15, 2024
129 checks passed

EgorBo deleted the opt-cast-cast-lookup branch February 15, 2024 19:55

DrewScoggins mentioned this pull request Feb 20, 2024

[Perf] Windows/x64: 4 Regressions on 2/16/2024 1:14:41 AM #98707

Closed

github-actions bot locked and limited conversation to collaborators Mar 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fold "cast(cast(obj, cls), cls)" to "cast(obj, cls)" #98337

Fold "cast(cast(obj, cls), cls)" to "cast(obj, cls)" #98337

EgorBo commented Feb 13, 2024

ghost commented Feb 13, 2024

EgorBo Feb 13, 2024

EgorBo Feb 13, 2024 •

edited

Loading

AndyAyersMS Feb 14, 2024

EgorBo Feb 14, 2024 •

edited

Loading

jkotas Feb 14, 2024

EgorBo Feb 14, 2024 •

edited

Loading

jkotas Feb 14, 2024

EgorBo commented Feb 13, 2024 •

edited

Loading

AndyAyersMS Feb 14, 2024

EgorBo Feb 14, 2024 •

edited

Loading

EgorBo commented Feb 14, 2024

Fold "cast(cast(obj, cls), cls)" to "cast(obj, cls)" #98337

Fold "cast(cast(obj, cls), cls)" to "cast(obj, cls)" #98337

Conversation

EgorBo commented Feb 13, 2024

ghost commented Feb 13, 2024

EgorBo Feb 13, 2024

Choose a reason for hiding this comment

EgorBo Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

AndyAyersMS Feb 14, 2024

Choose a reason for hiding this comment

EgorBo Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

jkotas Feb 14, 2024

Choose a reason for hiding this comment

EgorBo Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

jkotas Feb 14, 2024

Choose a reason for hiding this comment

EgorBo commented Feb 13, 2024 • edited Loading

AndyAyersMS Feb 14, 2024

Choose a reason for hiding this comment

EgorBo Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

EgorBo commented Feb 14, 2024

EgorBo Feb 13, 2024 •

edited

Loading

EgorBo Feb 14, 2024 •

edited

Loading

EgorBo Feb 14, 2024 •

edited

Loading

EgorBo commented Feb 13, 2024 •

edited

Loading

EgorBo Feb 14, 2024 •

edited

Loading