Skip to content

Conversation

@AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Aug 4, 2024

Have SSA create initial SSA defs for all locals instead of just the live subset (most of the time these will end up being unused). Have VN give these initial SSA defs suitable initial value numbers.

This fixes the "missing ssa def" issue seen in #105667 (where the only use of a local is an USEASG which does not make the use live) and also allows for a bit more VN based dead stores.

We could claw back some of the TP by only creating these unused things on demand.

Fixes #105667.

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 4, 2024
@AndyAyersMS AndyAyersMS changed the title Fix105667alt Push initial defs and create initial def VNs for all tracked locals Aug 5, 2024
@AndyAyersMS AndyAyersMS changed the title Push initial defs and create initial def VNs for all tracked locals JIT: Push initial defs and create initial def VNs for all tracked locals Aug 5, 2024
@AndyAyersMS AndyAyersMS marked this pull request as ready for review August 5, 2024 16:02
@AndyAyersMS AndyAyersMS requested a review from jakobbotsch August 5, 2024 16:02
@AndyAyersMS
Copy link
Member Author

FYI @dotnet/jit-contrib
@jakobbotsch PTAL

@AndyAyersMS
Copy link
Member Author

I wonder if at some point VN dead stores could supplant the redundant zero inits phase....

@SingleAccretion
Copy link
Contributor

This (presumably) obsoletes this workaround:

if (isUse && !storeRemoved)
{
// SSA and VN treat "partial definitions" as true uses, so for this
// front-end liveness pass we must add them to the live set in case
// we failed to remove the dead store.
if (varDsc->lvTracked)
{
VarSetOps::AddElemD(this, life, varDsc->lvVarIndex);
}
if (varDsc->lvPromoted)
{
for (unsigned fieldIndex = 0; fieldIndex < varDsc->lvFieldCnt; fieldIndex++)
{
LclVarDsc* fieldVarDsc = lvaGetDesc(varDsc->lvFieldLclStart + fieldIndex);
if (fieldVarDsc->lvTracked)
{
VarSetOps::AddElemD(this, life, fieldVarDsc->lvVarIndex);
}
}
}
}

@AndyAyersMS
Copy link
Member Author

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

This (presumably) obsoletes this workaround:

Ah, that likely explains why the issue in #105667 wasn't more widespread -- partial defs by calls were bypassed in this special liveness update.

So we could extend that handling to cover calls too, as an alternative (and perhaps less risky) fix.

@jakobbotsch
Copy link
Member

Ah, that likely explains why the issue in #105667 wasn't more widespread -- partial defs by calls were bypassed in this special liveness update.

So we could extend that handling to cover calls too, as an alternative (and perhaps less risky) fix.

Are you planning to change to handle it there?

@AndyAyersMS
Copy link
Member Author

Ah, that likely explains why the issue in #105667 wasn't more widespread -- partial defs by calls were bypassed in this special liveness update.
So we could extend that handling to cover calls too, as an alternative (and perhaps less risky) fix.

Are you planning to change to handle it there?

Seems like a more surgical fix, so yeah let me do that.

We can consider this change for .NET 10.

@AndyAyersMS AndyAyersMS added this to the 10.0.0 milestone Aug 5, 2024
@AndyAyersMS AndyAyersMS added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 5, 2024
@AndyAyersMS
Copy link
Member Author

Going to hold off on this for now...

@jakobbotsch jakobbotsch removed their request for review March 20, 2025 20:01
@AndyAyersMS
Copy link
Member Author

Closing for now, may want to revisit in .NET 11.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2025
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 NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assertion failed 'm_stacks != nullptr' during 'SSA: insert phis'

3 participants