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: Mark swift error as busy before call definition RefPosition #101792

Merged
merged 3 commits into from
May 3, 2024

Conversation

jakobbotsch
Copy link
Member

The RefPosition we were inserting here was inserted too late to actually protect the call definition from being allocated into the error register.

Instead, we can just mark the existing RefTypeFixedReg created for the argument use as delay freed, which will have the intended effect of keeping the error register busy until after the call definition.

Noticed this while looking at the RefPositions in #101760 (comment).

The RefPosition we were inserting here was inserted too late to actually
protect the call definition from being allocated into the error
register.

Instead, we can just mark the existing `RefTypeFixedReg` created for the
argument use as delay freed, which will have the intended effect of
keeping the error register busy until after the call definition.
@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 May 2, 2024
Copy link
Contributor

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

@jakobbotsch
Copy link
Member Author

TP differences are because of inlining decision changes. No longer calling newRefPosition leads to MSVC not inlining it anymore in some other places.

Base: 15869486520, Diff: 15875365069, +0.0370%

17470572  : NA      : 51.20% : +0.1101% : private: class RefPosition * __cdecl LinearScan::newRefPosition(enum _regNumber_enum, unsigned int, enum RefType, struct GenTree *, unsigned __int64)          
2470255   : +3.51%  : 7.24%  : +0.0156% : public: void __cdecl LinearScan::buildIntervals<1>(void)                                                                                                       
-816037   : -15.06% : 2.39%  : -0.0051% : private: void __cdecl LinearScan::buildUpperVectorRestoreRefPosition(class Interval *, unsigned int, struct GenTree *, bool, unsigned int)                     
-13290980 : -10.92% : 38.95% : -0.0838% : private: class RefPosition * __cdecl LinearScan::newRefPosition(class Interval *, unsigned int, enum RefType, struct GenTree *, unsigned __int64, unsigned int)

We expect these decisions to be significantly altered by native PGO anyway, so I don't see much point in making any changes here.

@jakobbotsch jakobbotsch marked this pull request as ready for review May 2, 2024 19:21
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @amanasifkhalid @kunalspathak

Copy link
Member

@amanasifkhalid amanasifkhalid left a comment

Choose a reason for hiding this comment

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

LGTM -- might be a good idea to run outerloop and/or jitstressregs (I think I remember the latter exposing a case where LSRA would try to use the error register to hold the Swift call's address). Thanks!

@jakobbotsch
Copy link
Member Author

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

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jakobbotsch
Copy link
Member Author

Failures are #99810 and #101721

@jakobbotsch jakobbotsch merged commit 15e98e5 into dotnet:main May 3, 2024
175 of 179 checks passed
@jakobbotsch jakobbotsch deleted the swift-delay-free branch May 3, 2024 07:38
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
…net#101792)

The RefPosition we were inserting here was inserted too late to actually
protect the call definition from being allocated into the error
register.

Instead, we can just mark the existing `RefTypeFixedReg` created for the
argument use as delay freed, which will have the intended effect of
keeping the error register busy until after the call definition.
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
…net#101792)

The RefPosition we were inserting here was inserted too late to actually
protect the call definition from being allocated into the error
register.

Instead, we can just mark the existing `RefTypeFixedReg` created for the
argument use as delay freed, which will have the intended effect of
keeping the error register busy until after the call definition.
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 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.

2 participants