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: Fix liveness for dependently promoted TYP_LONGs on x86 #73562

Merged
merged 4 commits into from
Aug 9, 2022

Conversation

jakobbotsch
Copy link
Member

We were only handling uses of promoted locals when varTypeIsStruct(lcl)
was true. For TYP_LONG promoted locals on x86 it is not.

Fix #73559

We were only handling uses of promoted locals when varTypeIsStruct(lcl)
was true. For TYP_LONG promoted locals on x86 it is not.

Fix dotnet#73559
@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 Aug 8, 2022
@ghost ghost assigned jakobbotsch Aug 8, 2022
@ghost
Copy link

ghost commented Aug 8, 2022

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

Issue Details

We were only handling uses of promoted locals when varTypeIsStruct(lcl)
was true. For TYP_LONG promoted locals on x86 it is not.

Fix #73559

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

Copy link
Contributor

@SingleAccretion SingleAccretion left a comment

Choose a reason for hiding this comment

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

LGTM. I was fixing the same issues the other day for multi-reg longs and wondered whether the current state is correct. Turns out not.

There is also this code in ref counting which I think should use the same treatment:

if (varTypeIsStruct(lvType) && propagate)
{
// For promoted struct locals, increment lvRefCnt on its field locals as well.
if (promotionType == Compiler::PROMOTION_TYPE_INDEPENDENT ||
promotionType == Compiler::PROMOTION_TYPE_DEPENDENT)
{
for (unsigned i = lvFieldLclStart; i < lvFieldLclStart + lvFieldCnt; ++i)
{
comp->lvaTable[i].incRefCnts(weight, comp, state, false); // Don't propagate
}
}
}

Many of these conditions look a bit pointless, they could just be checking promotion type and/or lvPromoted directly, but that's not a 7.0 issue, naturally.

@jakobbotsch jakobbotsch added this to the 7.0.0 milestone Aug 8, 2022
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. I am wondering if these 2 places also need fixup?

if (varTypeIsStruct(varDsc) &&

if (varTypeIsStruct(lvType))

@jakobbotsch
Copy link
Member Author

LGTM. I am wondering if these 2 places also need fixup?

if (varTypeIsStruct(varDsc) &&

if (varTypeIsStruct(lvType))

I don't think it would hurt, but it is not necessary today. We never promote TYP_LONG parameters independently (first one) and there is a != TYP_STRUCT check in the code that follows that covers the second one. I will leave it to avoid rerunning CI.

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

@jakobbotsch
Copy link
Member Author

Actually, CI looked particularly fussy so I updated them too to also rerun it.

@jakobbotsch jakobbotsch merged commit acc4f23 into dotnet:main Aug 9, 2022
@jakobbotsch jakobbotsch deleted the fix-73559 branch August 9, 2022 12:48
@ghost ghost locked as resolved and limited conversation to collaborators Sep 8, 2022
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
3 participants