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.

Sign up for GitHub

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

Jump to bottom

Liveness fix for struct enreg. #51851

Merged
merged 3 commits into from
Apr 28, 2021
Merged

Liveness fix for struct enreg. #51851

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba46901
Improve mustInit for structs.
Apr 26, 2021
5284943
fix linux
Apr 27, 2021
a9738ed
review response.
Apr 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions src/coreclr/jit/codegencommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4794,14 +4794,41 @@ void CodeGen::genCheckUseBlockInit()
}
}

/* With compInitMem, all untracked vars will have to be init'ed */
/* VSW 102460 - Do not force initialization of compiler generated temps,
unless they are untracked GC type or structs that contain GC pointers */
CLANG_FORMAT_COMMENT_ANCHOR;
bool mustInitThisVar = false;

const bool isTemp = varDsc->lvIsTemp;
const bool hasGCPtr = varDsc->HasGCPtr();
const bool isTracked = varDsc->lvTracked;
const bool isStruct = varTypeIsStruct(varDsc);
const bool compInitMem = compiler->info.compInitMem;
sandreenko marked this conversation as resolved.
Show resolved Hide resolved

if ((!varDsc->lvTracked || (varDsc->lvType == TYP_STRUCT)) && varDsc->lvOnFrame)
Copy link
Member

@erozenfeld erozenfeld Apr 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The new logic doesn't take into account varDsc->lvOnFrame. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, if I add an assert like if (mustInitThisVar) assert(varDsc->lvOnFrame); it won't fail (x64 spmi) but it is a current implicit contract so we probably should not rely on it.

if (hasGCPtr && !isTracked) -> not tracked means not in register -> lvOnFrame == true
else if (hasGCPtr && isStruct) -> do not enregister structs, with https://github.com/dotnet/runtime/pull/51850/checks?check_run_id=2447290001 won't enregister structs with GC pointers -> lvOnFrame == true
if (!isTracked)  -> not tracked means not in register -> lvOnFrame == true

I will return the condition, thank you.

if (hasGCPtr && !isTracked)
{
JITDUMP("must init V%02u because it has a GC ref\n", varNum);
mustInitThisVar = true;
}
else if (hasGCPtr && isStruct)
{
// TODO-1stClassStructs: support precise liveness reporting for such structs.
JITDUMP("must init a tracked V%02u because it a struct with a GC ref\n", varNum);
mustInitThisVar = true;
}
else
{
// We are done with tracked or GC vars, now look at untracked vars without GC refs.
if (!isTracked)
{
assert(!hasGCPtr);
if (compInitMem && !isTemp)
sandreenko marked this conversation as resolved.
Show resolved Hide resolved
{
JITDUMP("must init V%02u because compInitMem is set and it is not a temp\n", varNum);
mustInitThisVar = true;
}
}
}

if (mustInitThisVar)
{
varDsc->lvMustInit = true;

if (!counted)
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -971,7 +971,7 @@ class LclVarDsc
var_types lvaArgType();

// Returns true if this variable contains GC pointers (including being a GC pointer itself).
bool HasGCPtr()
bool HasGCPtr() const
{
return varTypeIsGC(lvType) || ((lvType == TYP_STRUCT) && m_layout->HasGCPtr());
}
Expand Down
22 changes: 20 additions & 2 deletions src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5915,17 +5915,35 @@ void Lowering::CheckNode(Compiler* compiler, GenTree* node)
{
unsigned lclNum = node->AsLclVarCommon()->GetLclNum();
LclVarDsc* lclVar = &compiler->lvaTable[lclNum];
#ifdef DEBUG
if (node->TypeIs(TYP_SIMD12))
{
assert(compiler->lvaIsFieldOfDependentlyPromotedStruct(lclVar) || (lclVar->lvSize() == 12));
}
#endif // DEBUG
}
break;
#endif // TARGET_64BIT
#endif // SIMD

case GT_LCL_VAR_ADDR:
case GT_LCL_FLD_ADDR:
{
const GenTreeLclVarCommon* lclVarAddr = node->AsLclVarCommon();
const LclVarDsc* varDsc = compiler->lvaGetDesc(lclVarAddr);
if (((lclVarAddr->gtFlags & GTF_VAR_DEF) != 0) && varDsc->HasGCPtr())
{
// Emitter does not correctly handle live updates for LCL_VAR_ADDR
// when they are not contained, for example, `STOREIND byref(GT_LCL_VAR_ADDR not-contained)`
// would generate:
// add r1, sp, 48 // r1 contains address of a lclVar V01.
// str r0, [r1] // a gc ref becomes live in V01, but emitter would not report it.
// Make sure that we use uncontained address nodes only for variables
// that will be marked as mustInit and will be alive throught the whole block even when tracked.
sandreenko marked this conversation as resolved.
Show resolved Hide resolved
assert(lclVarAddr->isContained() || !varDsc->lvTracked || varTypeIsStruct(varDsc));
// TODO: support this assert for uses, see https://github.com/dotnet/runtime/issues/51900.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#51900

}
break;
}

default:
break;
}
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/jit/optimizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9760,7 +9760,8 @@ void Compiler::optRemoveRedundantZeroInits()
if (BitVecOps::IsMember(&bitVecTraits, zeroInitLocals, lclNum) ||
(lclDsc->lvIsStructField &&
BitVecOps::IsMember(&bitVecTraits, zeroInitLocals, lclDsc->lvParentLcl)) ||
(!lclDsc->lvTracked && !fgVarNeedsExplicitZeroInit(lclNum, bbInALoop, bbIsReturn)))
((!lclDsc->lvTracked || !isEntire) &&
!fgVarNeedsExplicitZeroInit(lclNum, bbInALoop, bbIsReturn)))
{
// We are guaranteed to have a zero initialization in the prolog or a
// dominating explicit zero initialization and the local hasn't been redefined
Expand Down