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: Late expansion for casts #97075

Merged
merged 17 commits into from
Jan 19, 2024
Merged

JIT: Late expansion for casts #97075

merged 17 commits into from
Jan 19, 2024

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jan 17, 2024

Move cast expansion from importer to a late phase. For now, only for profiled isinst.

Closes #96837
Contributes to #84025

@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 17, 2024
@ghost ghost assigned EgorBo Jan 17, 2024
@ghost
Copy link

ghost commented Jan 17, 2024

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

Issue Details

Move cast expansion from importer to a late phase. For now, only for profiled isinst.

Closes #96837
Contributes to #84025

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo
Copy link
Member Author

EgorBo commented Jan 17, 2024

/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@EgorBo EgorBo marked this pull request as ready for review January 18, 2024 04:21
@EgorBo
Copy link
Member Author

EgorBo commented Jan 18, 2024

@jakobbotsch @dotnet/jit-contrib PTAL. Diffs. There are few things in the diffs:

  1. Improvements where we were able to hoist/CSE casts.

  2. Improvements where we realized block is cold and it doesn't need cast expansion, E.g.
    image
    (it was previously expanded)

  3. Regressions due to extra nullcheck this phase emits and no other phase can clean it up. But I already have a follow up patch to fix it via assertionprop

  4. Possible regressions because of CSE'd class handle argument - I wasn't able to find them but it may happen. This phase works as is if I move it after assertionprop so I can rely on VN to get it. Probably will do that separately.

Will move more casts to this phase from importer (all of them hopefully). For now only recently introduced profiled casts are handled.

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM, but looks like CI is a bit unhappy.

@EgorBo
Copy link
Member Author

EgorBo commented Jan 19, 2024

/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Jan 19, 2024

LGTM, but looks like CI is a bit unhappy.

Ah, I broke the control flow a bit - nullcheckBb used to jump straight to the use block without assigning tmp's value (but in most cases it was zeroed anyway so CI mostly passed.

Instead, I now do:

Case 1:

tmp = obj;
if (tmp != null)
{
    if (tmp.pMT != likelyClass)
        tmp = helperCall
    else
        no-op; // this block will be collected
}
use(tmp);

Case 2 (likelyClass is known to never pass the type check):

tmp = obj;
if (tmp != null)
{
    if (tmp.pMT != likelyClass)
        tmp = helperCall
    else
        tmp = null;
}
use(tmp);

I can avoid the initial tmp = obj; by introducing an extra BB but I think this shape is simpler.

@EgorBo
Copy link
Member Author

EgorBo commented Jan 19, 2024

Failure is #97103

@EgorBo EgorBo merged commit 26d8316 into dotnet:main Jan 19, 2024
127 of 129 checks passed
@EgorBo EgorBo deleted the late-cast-expansion branch January 19, 2024 20:59
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
@github-actions github-actions bot locked and limited conversation to collaborators Feb 19, 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.

Profiled casts: isinst is no longer hoisted
2 participants