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

Refactoring store liveness update #80539

Merged

Conversation

BrianBohe
Copy link
Member

Following up 79182. When storing a variable means a spill, we are updating its liveness and setting the register to REG_STK. When the target is a register and not the stack, then we usually call genProduceReg which triggers the liveness update. This pr refactor similar code to avoid unintentional changes breaking either var home or var liveness and fix one case on arm 64 which was not considered (emitIns_S_R and emitIns_R_I don't update var home).

@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 12, 2023
@ghost ghost assigned BrianBohe Jan 12, 2023
@ghost
Copy link

ghost commented Jan 12, 2023

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

Issue Details

Following up 79182. When storing a variable means a spill, we are updating its liveness and setting the register to REG_STK. When the target is a register and not the stack, then we usually call genProduceReg which triggers the liveness update. This pr refactor similar code to avoid unintentional changes breaking either var home or var liveness and fix one case on arm 64 which was not considered (emitIns_S_R and emitIns_R_I don't update var home).

Author: BrianBohe
Assignees: BrianBohe
Labels:

area-CodeGen-coreclr

Milestone: -

@BrianBohe
Copy link
Member Author

cc @dotnet/jit-contrib

@BrianBohe
Copy link
Member Author

@kunalspathak are you able to take a look at this pr? All errors seem to be known issues.

@kunalspathak
Copy link
Member

Seems like this affects the TP a bit. Can you please verify to see where it is coming from? Perhaps mark the method genUpdateLifeStore() with forceinline?

image

@BrianBohe
Copy link
Member Author

There is no difference on TP when using __forceinline, either compiling with /p:NoPgoOptimize=true or without.

@kunalspathak
Copy link
Member

Can you merge main to get the fix for #82506 and check the superpmi jobs?

@kunalspathak
Copy link
Member

Changes looks good to me. Once we get the superpmi numbers, we can go ahead and merge.

@BrianBohe
Copy link
Member Author

Changes looks good to me. Once we get the superpmi numbers, we can go ahead and merge.

TP is overall positive:
image

There is a timeout in OSX-x64 which seems to be non-related

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

@kunalspathak kunalspathak merged commit e3beb0e into dotnet:main Mar 1, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 31, 2023
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