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

LSRA: PreferCalleeSave for non-localVar intervals #92744

Closed
wants to merge 6 commits into from

Conversation

kunalspathak
Copy link
Member

Idea from #76904, but just with lower TP cost.

@ghost ghost assigned kunalspathak Sep 27, 2023
@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 Sep 27, 2023
@ghost
Copy link

ghost commented Sep 27, 2023

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

Issue Details

Idea from #76904, but just with lower TP cost.

Author: kunalspathak
Assignees: kunalspathak
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member

Is this different from #81242?

@kunalspathak
Copy link
Member Author

Is this different from #81242?

Didn't realize you had this already. Wondering why this was not merged. The idea is essentially same, but the way I was doing it in #76904 was TP heavy, so this should have lower TP impact. Compared to what you are doing, we might over mark preferCalleeSave to some intervals that has lastUse before the call and new definition after the call. Probably ok to not mark such intervals as preferCalleeSave?

RefTypeDef
RefTypeUse
RefTypeUse
RefTypeKill
RefTypeKill
...
RefTypeDef
RefTypeUse

@jakobbotsch
Copy link
Member

Wondering why this was not merged.

I think I wanted to investigate why this was a problem for floats but not for integer intervals, but I never got around to doing that. It's sort of odd that even without this preferencing we can allocate integer intervals without hitting the issue. Also IIRC the diffs were somewhat mixed.

This fix seems fine to me too, but the nice part about doing it from buildKillPositionsForNode is that we handle the local intervals and the LIR temp intervals from the same place. Not sure which one is best TP wise, though.

@ghost
Copy link

ghost commented Oct 27, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@kunalspathak
Copy link
Member Author

The results are mixed, and for some collection worse. There is around 0.5% regression in TP.

image

The approach I am taking here is if an interval ever interferes with call, I am marking it as preferCalleeSave (which is too aggressive) vs. what @jakobbotsch is doing in #81242 to just mark intervals whose definition is before call and use is after the call (LIR temps). I looked at the regressions and they are mostly coming from the extra prolog/epilog save restore of callee-save registers because they are preferred more now. We might see benefits of eliminating the memory access from the inner blocks from this and move that cost to prolog/epilog.

image

I tried something in the middle, where we mark those intervals to be preferCalleeSave whose def (and zero or more uses) is before the call and lastUse is after the call. That too gives mixed results, where the regressions are mostly from prolog/epilog save/restore.

@ghost ghost closed this Dec 8, 2023
@ghost
Copy link

ghost commented Dec 8, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2024
This pull request was closed.
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